Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report discarded notification observer as errors #20973

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented Jun 29, 2023

Fixes #20994.

This PR adds the discarded_notification_center_observer SwiftLint rule to this repo's SwiftLint configuration. All existing violations have been fixed (see the referenced PR in the issue). And from now all, new violations will be reported as errors.

As mentioned in the issue description. Observers not getting unregistered may cause problems because

  • The block argument will not be released, which causes some objects not being released in some cases.
  • The observer code is executed at a time that you don't expect them to execute.

Regression Notes

N/A

PR submission checklist: N/A

UI Changes testing checklist: N/A

@crazytonyli crazytonyli force-pushed the discarded-notification-observers branch from 8eb366c to 4e487f3 Compare June 29, 2023 22:41
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 29, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr20973-bddf5db
Version22.8
Bundle IDorg.wordpress.alpha
Commitbddf5db
App Center BuildWPiOS - One-Offs #6479
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@crazytonyli crazytonyli force-pushed the discarded-notification-observers branch from 4e487f3 to 04a9c6b Compare July 19, 2023 03:00
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 19, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr20973-bddf5db
Version22.8
Bundle IDcom.jetpack.alpha
Commitbddf5db
App Center Buildjetpack-installable-builds #5511
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@crazytonyli crazytonyli marked this pull request as ready for review July 19, 2023 04:02
@crazytonyli crazytonyli requested review from jkmassel and mokagio July 19, 2023 04:02
@crazytonyli crazytonyli enabled auto-merge July 19, 2023 04:04
@crazytonyli crazytonyli self-assigned this Jul 19, 2023
@crazytonyli crazytonyli added this to the 22.9 milestone Jul 19, 2023
@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@crazytonyli crazytonyli merged commit c0a6b00 into trunk Jul 24, 2023
@crazytonyli crazytonyli deleted the discarded-notification-observers branch July 24, 2023 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some notification observers are not removed
3 participants