Skip to content

Report discarded notification observer as errors#20973

Merged
crazytonyli merged 2 commits intotrunkfrom
discarded-notification-observers
Jul 24, 2023
Merged

Report discarded notification observer as errors#20973
crazytonyli merged 2 commits intotrunkfrom
discarded-notification-observers

Conversation

@crazytonyli
Copy link
Copy Markdown
Contributor

@crazytonyli crazytonyli commented Jun 29, 2023 •
edited
Loading

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
Copy Markdown
Contributor

wpmobilebot commented Jun 29, 2023 •
edited
Loading

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
Copy Markdown
Contributor

wpmobilebot commented Jul 19, 2023 •
edited
Loading

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
Copy Markdown

peril-wordpress-mobile bot commented Jul 23, 2023

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

mokagio
mokagio approved these changes Jul 24, 2023
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@mokagio mokagio mokagio approved these changes

@jkmassel jkmassel Awaiting requested review from jkmassel

Assignees

@crazytonyli crazytonyli

Labels

None yet

Projects

None yet

Milestone

22.9

Development

Successfully merging this pull request may close these issues.

Some notification observers are not removed

3 participants

@crazytonyli @wpmobilebot @mokagio