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

Make AbstractConfig collection fields explicity Immutable #601

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

lazaroclapp
Copy link
Collaborator

Follow up to #600. These values are already ImmutableSet at runtime
(except one which was still used strictly as read-only). This change just
makes it explicit.

/**
* if true, {@link #fromAnnotatedPackage(Symbol.ClassSymbol)} will return false for any class
* annotated with {@link javax.annotation.Generated}
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No other such protected field has docs and besides these are stale (#fromAnnotatedPackage for this class no longer exists, it was never only that annotation, and after #600 this isn't even only [simple-name] @Generated), so...

@@ -296,15 +292,15 @@ public boolean isContractAnnotation(String annotationName) {
return contractAnnotations.contains(annotationName);
}

protected Set<MethodClassAndName> getKnownInitializers(Set<String> qualifiedNames) {
protected ImmutableSet<MethodClassAndName> getKnownInitializers(Set<String> qualifiedNames) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note this is called only once during config loading in ErrorProneCLIFlagsConfig

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

👍

@lazaroclapp lazaroclapp requested a review from msridhar May 13, 2022 23:23
@coveralls
Copy link

coveralls commented May 13, 2022

Pull Request Test Coverage Report for Build #954

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.004%) to 92.523%

Files with Coverage Reduction New Missed Lines %
../nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java 4 93.75%
../nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java 4 93.9%
Totals Coverage Status
Change from base Build #953: -0.004%
Covered Lines: 4925
Relevant Lines: 5323

💛 - Coveralls

This is a re-do, from scratch of a very stale PR.
@lazaroclapp lazaroclapp force-pushed the lazaro_nullaway_change_config_sets_to_immutable branch from 7df44b4 to 9fbea32 Compare September 13, 2022 17:56
@lazaroclapp
Copy link
Collaborator Author

@msridhar re-did this PR because it's bothering me that we have these very old small PRs in perpetual "under review" state 😅 , let's get open-PRs to zero if we can. Not sure if #467 can easily be updated and landed (or if it should be abandoned), though.

@lazaroclapp lazaroclapp merged commit a7ba27e into master Sep 13, 2022
@lazaroclapp lazaroclapp deleted the lazaro_nullaway_change_config_sets_to_immutable branch September 13, 2022 18:30
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.

3 participants