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

UnusedMethod: Support additional exempting method annotations #34

Closed
wants to merge 2 commits into from
Closed

UnusedMethod: Support additional exempting method annotations #34

wants to merge 2 commits into from

Conversation

rickie
Copy link
Member

@rickie rickie commented Jun 30, 2022

This solves two open issues (which can be linked if the PR is in a good state).

@rickie rickie requested a review from Stephan202 June 30, 2022 11:39
@rickie rickie force-pushed the rossendrijver/unusedmethod_improvements branch from 3d2afe1 to 5f8ba8d Compare June 30, 2022 11:42
rickie
rickie commented Jun 30, 2022
View reviewed changes
core/src/main/java/com/google/errorprone/bugpatterns/UnusedMethod.java Outdated

/** The set of types exempting a type that is extending or implementing them. */
private static final ImmutableSet<String> EXEMPTING_SUPER_TYPES = ImmutableSet.of();

private final ImmutableSet<String> exemptMethodAnnotations;
Copy link
Member Author

Choose a reason for hiding this comment

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

An open question:

  • How do we want to name this variable? It is relatively similar to the setup in the UnusedVariable check.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should stick with the "exempting" terminology, as the annotations "influence" whether the methods are flagged.

@Stephan202 Stephan202 force-pushed the rossendrijver/unusedmethod_improvements branch from 5f8ba8d to 70431bb Compare August 17, 2022 20:34
@Stephan202 Stephan202 changed the title UnusedMethod: Add exempting method annotations and support providing arguments UnusedMethod: Support additional exempting method annotations Aug 17, 2022
Stephan202
Stephan202 approved these changes Aug 17, 2022
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.

Nice! Added a commit and tweaked the suggested PR title.

core/src/test/java/com/google/errorprone/bugpatterns/UnusedMethodTest.java Outdated
" @Foo",
" private void bar() {}",
"}")
.setArgs(ImmutableList.of("-XepOpt:UnusedMethod:ExemptMethodAnnotations=example.Foo"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.setArgs(ImmutableList.of("-XepOpt:UnusedMethod:ExemptMethodAnnotations=example.Foo"))
.setArgs("-XepOpt:UnusedMethod:ExemptMethodAnnotations=example.Foo")

core/src/test/java/com/google/errorprone/bugpatterns/UnusedMethodTest.java Outdated
@@ -138,6 +139,21 @@ public void exemptedByName() {
.doTest();
}

@Test
public void exemptedByPassingViaArgs() {
Copy link
Member

Choose a reason for hiding this comment

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

How about:

Suggested change
public void exemptedByPassingViaArgs() {
public void exemptedByCustomAnnotation() {

core/src/test/java/com/google/errorprone/bugpatterns/UnusedMethodTest.java Outdated
helper
.addSourceLines("Foo.java", "package example;", "@interface Foo {}")
.addSourceLines(
"ExemptedViaArgs.java",
Copy link
Member

Choose a reason for hiding this comment

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

Could do a similar rename here; will push a proposal.

core/src/main/java/com/google/errorprone/bugpatterns/UnusedMethod.java Outdated
Comment on lines 154 to 158
ImmutableSet.Builder<String> exemptMethodAnnotations = ImmutableSet.builder();
errorProneFlags
.getList("UnusedMethod:ExemptMethodAnnotations")
.ifPresent(exemptMethodAnnotations::addAll);
this.exemptMethodAnnotations = exemptMethodAnnotations.build();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ImmutableSet.Builder<String> exemptMethodAnnotations = ImmutableSet.builder();
errorProneFlags
.getList("UnusedMethod:ExemptMethodAnnotations")
.ifPresent(exemptMethodAnnotations::addAll);
this.exemptMethodAnnotations = exemptMethodAnnotations.build();
this.exemptMethodAnnotations =
errorProneFlags
.getList("UnusedMethod:ExemptMethodAnnotations")
.map(ImmutableSet::copyOf)
.orElseGet(ImmutableSet::of);

core/src/main/java/com/google/errorprone/bugpatterns/UnusedMethod.java Outdated

/** The set of types exempting a type that is extending or implementing them. */
private static final ImmutableSet<String> EXEMPTING_SUPER_TYPES = ImmutableSet.of();

private final ImmutableSet<String> exemptMethodAnnotations;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should stick with the "exempting" terminology, as the annotations "influence" whether the methods are flagged.

core/src/main/java/com/google/errorprone/bugpatterns/UnusedMethod.java Outdated
public UnusedMethod(ErrorProneFlags errorProneFlags) {
ImmutableSet.Builder<String> exemptMethodAnnotations = ImmutableSet.builder();
errorProneFlags
.getList("UnusedMethod:ExemptMethodAnnotations")
Copy link
Member

Choose a reason for hiding this comment

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

Arguably we can omit the second use of "Method", as that's implied.

Edit: but maybe later we also want to suppose class-level exempting methods, so perhaps better to keep it as-is.

@rickie
Copy link
Member Author

rickie commented Aug 30, 2022

We can squash the commits and use the suggested commit message? (current PR title)

UnusedMethod: Support additional exempting method annotations

One proposal/thing to consider, if we change the order of additional and exempting the focus is more on the part that supports passing in extra method annotations, whereas the current one focuses on the added method annotations.

@rickie rickie force-pushed the rossendrijver/unusedmethod_improvements branch 2 times, most recently from 11119bd to 10fcb9c Compare September 2, 2022 06:16
@rickie rickie force-pushed the rossendrijver/unusedmethod_improvements branch from 10fcb9c to a5d36e3 Compare September 26, 2022 11:11
@rickie rickie force-pushed the rossendrijver/unusedmethod_improvements branch from a5d36e3 to 99f8878 Compare December 15, 2022 15:19
@rickie
Copy link
Member Author

rickie commented Dec 27, 2022

Fixed in google#3428.

@rickie rickie closed this Dec 27, 2022
@Stephan202 Stephan202 deleted the rossendrijver/unusedmethod_improvements branch December 30, 2022 17:34
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