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.

Sign up for GitHub

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

Jump to bottom

ObserverManager: nits #8460

Merged
merged 1 commit into from Jun 5, 2023
Merged

ObserverManager: nits #8460

merged 1 commit into from Jun 5, 2023

Conversation

ScarletKuro
Copy link
Contributor

@ScarletKuro ScarletKuro commented Jun 2, 2023 •
edited

I made a few adjustments:

  • In the Unsubscribe method, I added a missing if clause with _log.IsEnabled(LogLevel.Debug) and moved the log message after the Remove call to ensure that the Count property gets updated correctly. Previously, the message could be misleading because it displayed the count before the removal.
  • I addressed the CA2253: Named placeholders should not be numeric values.
  • I also added a where TIdentity : notnull constraint. Although nullable is not enabled, the Dictionary uses this constraint, so I applied it for consistency.
  • Additionally, I replaced {Observer} with {Id} in one of the log message, as it aligns with the rest of the code and reflects the actual type of TIdentity.
Microsoft Reviewers: Open in CodeFlow

@ScarletKuro
Copy link
Contributor Author

@dotnet-policy-service agree

ReubenBond
ReubenBond approved these changes Jun 5, 2023
View reviewed changes
Copy link
Member

@ReubenBond ReubenBond left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@ReubenBond
Copy link
Member

ReubenBond commented Jun 5, 2023 •
edited

Will merge after #8468 is merged, since that fixes an issue related to observers which I introduced.

@ReubenBond ReubenBond self-assigned this Jun 5, 2023
@ReubenBond ReubenBond merged commit 45b357f into dotnet:main Jun 5, 2023
18 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ReubenBond ReubenBond ReubenBond approved these changes

Assignees

@ReubenBond ReubenBond

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ScarletKuro @ReubenBond