-
Notifications
You must be signed in to change notification settings - Fork 765
Have -XepPatchChecks:
skip disabled checks
#5102
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
Have -XepPatchChecks:
skip disabled checks
#5102
Conversation
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.
Some context.
ImmutableSet<String> disabled = | ||
getAllChecks().values().stream() | ||
.filter(predicate.negate()) | ||
.filter(bci -> disabled().contains(bci.canonicalName()) || !predicate.test(bci)) |
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.
The changes in #4699 assumed that ScannerSupplier#filter
would merely disable additional checks, but in fact it could re-enable disabled checks. This change proposes to resolve that, yielding more intuitive semantics.
@Test | ||
public void patchAllWithDisableAllChecks() throws IOException { | ||
JavaFileObject fileObject = | ||
createOnDiskFileObject( | ||
"StringConstantWrapper.java", | ||
""" | ||
class StringConstantWrapper { | ||
String s = "old-value"; | ||
} | ||
"""); | ||
|
||
CompilationResult result = | ||
doCompile( | ||
Collections.singleton(fileObject), | ||
Arrays.asList("-XepPatchChecks:", "-XepPatchLocation:IN_PLACE", "-XepDisableAllChecks"), | ||
ImmutableList.of(AssignmentUpdater.class)); | ||
assertThat(result.succeeded).isTrue(); | ||
assertThat(Files.readString(Path.of(fileObject.toUri()))) | ||
.isEqualTo( | ||
""" | ||
class StringConstantWrapper { | ||
String s = "old-value"; | ||
} | ||
"""); | ||
} |
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.
This new test fails prior to the proposed changes. Observe that the only difference with the preceding patchAllWithCheckDisabled
test is the use of -XepDisableAllChecks
instead of -Xep:AssignmentUpdater:OFF
.
Thanks for the fix! |
Would it be possible to merge this fix? |
+1 This is particularly annoying, would love to see this merged. |
Resolves #4943.