-
Notifications
You must be signed in to change notification settings - Fork 39
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 BugPattern{,Test}Extractor
tests more maintainable
#937
Make BugPattern{,Test}Extractor
tests more maintainable
#937
Conversation
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
static BugPatternDocumentation create( | ||
URI source, | ||
String fullyQualifiedName, | ||
String name, | ||
ImmutableList<String> altNames, | ||
String link, | ||
ImmutableList<String> tags, | ||
String summary, | ||
String explanation, | ||
SeverityLevel severityLevel, | ||
boolean canDisable, | ||
ImmutableList<String> suppressionAnnotations) { | ||
return new AutoValue_BugPatternExtractor_BugPatternDocumentation( | ||
source, | ||
fullyQualifiedName, | ||
name, | ||
altNames, | ||
link, | ||
tags, | ||
summary, | ||
explanation, | ||
severityLevel, | ||
canDisable, | ||
suppressionAnnotations); | ||
} |
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 is boilerplate, but Error Prone flags new AutoValue_BugPatternExtractor_BugPatternDocumentation
(and similar) calls outside the class in which the immutable type is defined.
/** | ||
* Utility class that offers mutually consistent JSON serialization and deserialization operations, | ||
* without further specifying the exact schema used. | ||
*/ | ||
final class Json { | ||
private static final ObjectMapper OBJECT_MAPPER = | ||
new ObjectMapper() | ||
.setVisibility(PropertyAccessor.FIELD, Visibility.ANY) | ||
.registerModules(new GuavaModule(), new ParameterNamesModule()); |
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 logic is moved to a separate class, not only because the tests use it, but also because other yet-to-backported code relies on it.
BugPatternDocumentation.create( | ||
URI.create("file:///MinimalBugChecker.java"), | ||
"pkg.MinimalBugChecker", | ||
"MinimalBugChecker", | ||
ImmutableList.of(), | ||
"", | ||
ImmutableList.of(), | ||
"MinimalBugChecker summary", | ||
"", | ||
ERROR, | ||
/* canDisable= */ true, | ||
ImmutableList.of(SuppressWarnings.class.getCanonicalName()))); |
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.
Here and below: these objects replace the JSON resource files.
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.
Nothing stands out to me 🚀
Only one comment about field order, but I am fine as-is ✅
...entation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternExtractor.java
Show resolved
Hide resolved
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.
Tnx for the quick review @mohamedsamehsalah! 🚀
I added one more commit, as I forgot to include a test class.
...entation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternExtractor.java
Show resolved
Hide resolved
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
💯
} | ||
|
||
@FormatMethod | ||
private static UncheckedIOException failure(IOException cause, String format, Object... args) { |
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 it make sense to have the same ordering as creating the exception, so having cause
last?
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.
Well, Java requires that the varargs argument is last (to avoid ambiguity). :)
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.
😂 oh ofcourse, forgot that. Let's merge!
As we're moving to a Java-based website generator located in the same package as the `Extractor` implementations, there is no need to validate the exact format of generated files; only that the data can be deserialized again. While there, track the source file from which data is extracted.
56833db
to
9a0ad2b
Compare
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
These change are backported from the
website
branch.Suggested commit message: