-
Notifications
You must be signed in to change notification settings - Fork 737
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
Add option to ignore missing @Override annotations in interfaces #926
Add option to ignore missing @Override annotations in interfaces #926
Conversation
… in interfaces
.doTest(); | ||
} | ||
|
||
// this doesn't work! need new APIs in CompilationTestHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would appreciate some advice on how to test. The most obvious route I could was to add APIs to CompilationTestHelper
to allow for passing in custom ErrorProneFlags
. But I wanted to check if there is an easier way before going ahead with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why doesn't this work? In some private code I do the following, which does work:
@RunWith(JUnit4.class)
public final class LexicographicalAnnotationAttributeListingCheckTest {
...
private final CompilationTestHelper restrictedCompilationTestHelper =
CompilationTestHelper.newInstance(
LexicographicalAnnotationAttributeListingCheck.class, getClass())
.setArgs(
ImmutableList.of(
"-XepOpt:LexicographicalAnnotationAttributeListing:Includes=pkg.A.Foo,pkg.A.Bar",
"-XepOpt:LexicographicalAnnotationAttributeListing:Excludes=pkg.A.Bar#value"));
...
@Test
public void testFiltering() {
restrictedCompilationTestHelper
.addSourceLines(...)
.doTest();
}
}
private final boolean ignoreInterfaceOverrides; | ||
|
||
public MissingOverride() { | ||
this.ignoreInterfaceOverrides = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could keep the default (i.e., false
) in one place by replacing this constructor implementation with this(ErrorProneFlags.empty());
.
.doTest(); | ||
} | ||
|
||
// this doesn't work! need new APIs in CompilationTestHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why doesn't this work? In some private code I do the following, which does work:
@RunWith(JUnit4.class)
public final class LexicographicalAnnotationAttributeListingCheckTest {
...
private final CompilationTestHelper restrictedCompilationTestHelper =
CompilationTestHelper.newInstance(
LexicographicalAnnotationAttributeListingCheck.class, getClass())
.setArgs(
ImmutableList.of(
"-XepOpt:LexicographicalAnnotationAttributeListing:Includes=pkg.A.Foo,pkg.A.Bar",
"-XepOpt:LexicographicalAnnotationAttributeListing:Excludes=pkg.A.Bar#value"));
...
@Test
public void testFiltering() {
restrictedCompilationTestHelper
.addSourceLines(...)
.doTest();
}
}
@@ -46,6 +47,20 @@ | |||
) | |||
public class MissingOverride extends BugChecker implements MethodTreeMatcher { | |||
|
|||
/** | |||
* if true, don't warn on missing @Override annotations inside interfaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe write {@code @Override}
, so as not to confuse the javadoc tool?
@Stephan202 thanks for the review! And you were right about the tests; I must have been invoking maven incorrectly before. |
@eaftan any thoughts on this one? |
@eaftan another bump on this one, if you have time |
Fixes google#926 MOE_MIGRATED_REVID=209197062
@cushon should I open a separate PR to update the docs? |
In the mobile RIB architecture, we often have cases where Dagger Component-related interfaces form an inheritance chain, such that a method gets added to a Component super-interface that was already present in the Component sub-interface. In such a case, the EP MissingOverride check forces an
@Override
annotation to be added to the sub-interface. Forcing this change goes against some of the modularity in the RIB architecture (@AttwellBrian can explain more). Further, in this case the annotation does not really add safety, as (I think) Dagger code gen will complain if there is some ambiguity in the set of methods exposed by the component sub-interface.In general, it's not clear whether forcing
@Override
annotations in interfaces is as much of a win as it is for classes. (Kotlin forces it on interfaces, but Swift only requires it on classes, not sub-protocols.) This PR adds an option to the MissingOverride check to disable checking for@Override
annotations in interfaces.