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

refactor: make Analyzer accept the diagnostics configuration. #274

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

peterhuene
Copy link
Collaborator

This commit refactors the new and new_with_validator methods of Analyzer to directly accept the DiagnosticsConfig rather than an iterator of rules which it immediately uses to create a DiagnosticsConfig internally.

By having the DiagnosticsConfig (which is Copy) passed in directly, we don't have to borrow a rule exception list that might also be used as part of the validator closure passed to new_with_validator, sparing us a clone.

Before submitting this PR, please make sure:

  • You have added a few sentences describing the PR here.
  • You have added yourself or the appropriate individual as the assignee.
  • You have added at least one relevant code reviewer to the PR.
  • Your code builds clean without any errors or warnings.
  • You have added tests (when appropriate).
  • You have updated the README or other documentation to account for these
    changes (when appropriate).
  • You have added an entry to the relevant CHANGELOG.md (see
    "keep a changelog" for more information).
  • Your commit messages follow the conventional commit style.

This commit refactors the `new` and `new_with_validator` methods of `Analyzer`
to directly accept the `DiagnosticsConfig` rather than an iterator of rules
which it immediately uses to create a `DiagnosticsConfig` internally.

By having the `DiagnosticsConfig` (which is `Copy`) passed in directly, we
don't have to borrow a rule exception list that might also be used as part of
the validator closure passed to `new_with_validator`, sparing us a clone.
Copy link
Member

@a-frantz a-frantz left a comment

Choose a reason for hiding this comment

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

Missing a few CHANGELOG entries, but otherwise LGTM

@peterhuene
Copy link
Collaborator Author

Oh I don't bother with updating CHANGELOG for cascading changes from an upstream API change, just the CHANGELOG of the crate with the change itself.

@a-frantz
Copy link
Member

O ok. That makes sense to me now that you say it! Assuming gauntlet on windows turns green, this gets my approval then

@peterhuene peterhuene merged commit 8c0fb17 into stjude-rust-labs:main Dec 18, 2024
16 checks passed
@peterhuene peterhuene deleted the fix-analyzer branch December 18, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants