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

CanIgnoreReturnValueSuggester: Support exempting method annotations #60

Closed
wants to merge 1 commit into from
Closed

CanIgnoreReturnValueSuggester: Support exempting method annotations #60

wants to merge 1 commit into from

Conversation

rickie
Copy link
Member

@rickie rickie commented May 13, 2023 •
edited
Loading

While there, exempt @AfterTemplate methods from analysis.

@rickie rickie requested a review from Stephan202 May 13, 2023 15:41
@rickie rickie mentioned this pull request May 13, 2023
Merged
1 task
@rickie rickie force-pushed the rossendrijver/CIRV_Refaster branch from f6221f0 to 2ae71e5 Compare May 27, 2023 09:59
@rickie rickie changed the title Dont flag @AfterTemplate methods with @CIRV CanIgnoreReturnValueSuggester: Support additional exempting method annotations May 27, 2023
@rickie rickie changed the title CanIgnoreReturnValueSuggester: Support additional exempting method annotations CanIgnoreReturnValueSuggester: Support exempting method annotations May 27, 2023
@rickie
Copy link
Member Author

rickie commented Jun 7, 2023 •
edited by Stephan202
Loading

Suggested commit message:

CanIgnoreReturnValueSuggester: Support custom exempting method annotations

While there, exempt `@AfterTemplate` methods from analysis.

@rickie rickie force-pushed the rossendrijver/CIRV_Refaster branch from 2ae71e5 to eb0f56d Compare June 7, 2023 17:45
@Stephan202 Stephan202 force-pushed the rossendrijver/CIRV_Refaster branch from eb0f56d to a37e503 Compare July 2, 2023 12:59
Stephan202
Stephan202 approved these changes Jul 2, 2023
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Rebased and added a commit. One can debate whether @CRV and @CIRV are "exempting" method annotations, but they do prevent this checker for making a suggestion 🤷

Slightly tweaked the suggested commit message.

...n/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java Outdated

// If the method is annotated with an annotation that is exempted, bail out.
if (additionalExemptingMethodAnnotations.stream()
.anyMatch(annotation -> ASTHelpers.hasAnnotation(methodTree, annotation, state))) {
Copy link
Member

Choose a reason for hiding this comment

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

We can:

  1. Statically import this method.
  2. Use the MethodSymbol-accepting overload used above.
  3. Use this anyMatch also for the cases mentioned above.

@rickie rickie force-pushed the rossendrijver/CIRV_Refaster branch 2 times, most recently from 2a87390 to 454bdd4 Compare July 11, 2023 07:03
@rickie
Copy link
Member Author

rickie commented Jul 11, 2023

Upstream PR: google#4009.

@rickie
CanIgnoreReturnValueSuggester: Support custom exempting method annota…
fd25f02 
…tions

While there, excempt `@AfterTemplate` methods with analysis.
@rickie rickie force-pushed the rossendrijver/CIRV_Refaster branch from 454bdd4 to fd25f02 Compare August 29, 2023 09:19
@Stephan202
Copy link
Member

Upstream PR was merged 👍

@Stephan202 Stephan202 closed this Sep 13, 2023
@Stephan202 Stephan202 deleted the rossendrijver/CIRV_Refaster branch September 13, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stephan202 Stephan202 Stephan202 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rickie @Stephan202