-
Notifications
You must be signed in to change notification settings - Fork 36
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
Update BugPattern#{link,linkType.CUSTOM}
to link to our website
#251
Conversation
BugPattern#{link,linkType.CUSTOM}
to link to our websiteBugPattern#{link,linkType.CUSTOM}
to link to our website
README.md
Outdated
@@ -121,7 +121,8 @@ $ mvn clean install | |||
[INFO] ------------------------------------------------------------- | |||
[WARNING] Example.java:[9,34] [tech.picnic.errorprone.refastertemplates.BigDecimalTemplates.BigDecimalZero] | |||
Did you mean 'return BigDecimal.ZERO;'? | |||
[WARNING] Example.java:[14,35] [IdentityConversion] This method invocation appears redundant; remove it or suppress this warning and add a comment explaining its purpose | |||
[WARNING] Example.java:[13,35] [IdentityConversion] This method invocation appears redundant; remove it or suppress this warning and add a comment explaining its purpose | |||
(see https://error-prone.picnic.tech/bug_patterns/IdentityConversion) |
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.
😍
@@ -26,7 +26,8 @@ | |||
@AutoService(BugChecker.class) | |||
@BugPattern( | |||
summary = "`JsonCreator.Mode` should be set for single-argument creators", | |||
linkType = NONE, | |||
link = "https://error-prone.picnic.tech/bug_patterns/AmbiguousJsonCreator", |
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.
Note: here and below, we're mixing camel and snake case here.
However, this is consistent with errorprone.info, e.g. http://errorprone.info/bugpattern/AndroidInjectionBeforeSuper
(Bug Pattern
-> bugpattern
is actually lowercase 🙄)
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.
@rickie is it possible to define the base url in a single place?
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.
However, this is consistent with errorprone.info
😂
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.
@rickie is it possible to define the base url in a single place?
Not without making a change directly in Error Prone to be honest.
This is the relevant method in EP:
@Nullable
private static String createLinkUrl(String canonicalName, BugPattern pattern) {
switch (pattern.linkType()) {
case AUTOGENERATED:
return String.format("https://errorprone.info/bugpattern/%s", canonicalName);
case CUSTOM:
// annotation.link() must be provided.
if (pattern.link().isEmpty()) {
throw new IllegalStateException(
"If linkType element of @BugPattern is CUSTOM, "
+ "a link element must also be provided.");
}
return pattern.link();
case NONE:
return null;
}
throw new AssertionError(
"Unexpected value for linkType element of @BugPattern: " + pattern.linkType());
}
Introducing a new LinkType.PICNIC
would be cool though 😂. (Not an option though haha)
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.
We could perhaps introduce a static constant for the https://error-prone.picnic.tech/bug_patterns/
prefix in the tech.picnic.errorprone.bugpatterns.util
package. 🤔
W.r.t. the path: I think I prefer bugpattern
(for consistency with Error Prone) or bug-pattern
/bug-patterns
over bug_patterns
:)
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.
Forgot to push, did that here: 07a0034
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.
Nice!
@@ -26,7 +26,8 @@ | |||
@AutoService(BugChecker.class) | |||
@BugPattern( | |||
summary = "`JsonCreator.Mode` should be set for single-argument creators", | |||
linkType = NONE, | |||
link = "https://error-prone.picnic.tech/bug_patterns/AmbiguousJsonCreator", |
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.
We could perhaps introduce a static constant for the https://error-prone.picnic.tech/bug_patterns/
prefix in the tech.picnic.errorprone.bugpatterns.util
package. 🤔
W.r.t. the path: I think I prefer bugpattern
(for consistency with Error Prone) or bug-pattern
/bug-patterns
over bug_patterns
:)
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.
Just dropping a thought I just had while talking with @loehnertz :)
[WARNING] Example.java:[9,34] [tech.picnic.errorprone.refastertemplates.BigDecimalTemplates.BigDecimalZero] | ||
Did you mean 'return BigDecimal.ZERO;'? | ||
[WARNING] Example.java:[14,35] [IdentityConversion] This method invocation appears redundant; remove it or suppress this warning and add a comment explaining its purpose | ||
[WARNING] Example.java:[13,35] [IdentityConversion] This method invocation appears redundant; remove it or suppress this warning and add a comment explaining its purpose | ||
(see https://error-prone.picnic.tech/bugpatterns/IdentityConversion) |
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.
For a separate PR: we should also emit these links for Refaster-based checks :)
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.
Means extending refaster, right? AFAIK not supported by error-prone
, right?
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.
Indeed; out of the box Refaster only patches. I filed #255 to track this.
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.
@Stephan202 super! 💪
Suggested commit message:
|
@@ -121,7 +121,8 @@ $ mvn clean install | |||
[INFO] ------------------------------------------------------------- | |||
[WARNING] Example.java:[9,34] [tech.picnic.errorprone.refastertemplates.BigDecimalTemplates.BigDecimalZero] | |||
Did you mean 'return BigDecimal.ZERO;'? | |||
[WARNING] Example.java:[14,35] [IdentityConversion] This method invocation appears redundant; remove it or suppress this warning and add a comment explaining its purpose | |||
[WARNING] Example.java:[13,35] [IdentityConversion] This method invocation appears redundant; remove it or suppress this warning and add a comment explaining its purpose | |||
(see https://error-prone.picnic.tech/bugpatterns/IdentityConversion) |
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: we have URLs without trailing slashes, which is consistent with the rest of this and other of our codebases.
There are many opinions what's the right approach. Generally there seems some consensus where /name/
refers to a directory and /name
refers to file (albeit weirdly without an extensions). Jekyll, however, currently uses trailing slashes and it does not seem we can configure it as such to omit them (even when doing so, it redirects the page to include a trailing slash).
Given that both work, I'd suggest leaving the URLs like this. If we manage for Jekyll to omit the trailing slash, these URLs will continue to work.
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.
SGTM. Also I think for a URL it looks nicer to not have a trailing slash.
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.
That's super subjective 😉 (although I agree)
@@ -26,7 +27,8 @@ | |||
@AutoService(BugChecker.class) | |||
@BugPattern( | |||
summary = "`JsonCreator.Mode` should be set for single-argument creators", | |||
linkType = NONE, | |||
link = BASE_URL_BUGPATTERNS + "AmbiguousJsonCreator", |
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.
I assume we can't auto-generate this based on the class name?
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.
Should we use URI.resolve
here instead?
link = BASE_URL_BUGPATTERNS + "AmbiguousJsonCreator", | |
link = BASE_URL_BUGPATTERNS.resolve("AmbiguousJsonCreator"), |
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.
We can indeed not auto-generate.
link
should be a String constant. The proposed solution is an URI and not a constant. .toString()
would solve the former issue but not the latter 😬.
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.
Clear!
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/Documentation.java
Outdated
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.
I verified that all the URLs work 💪
https://error-prone.picnic.tech/bugpatterns/
+
grep -r -l '@BugPattern' error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns | xargs -I {} basename {} .java
b68ba62
to
b5dc7e3
Compare
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.
Rebased and added a commit. Not yet approving because I want to asses the effort required to automate/enforce this (started playing with something).
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.
Rebased and added a commit to update a recently introduced bug checker.
Rebased and added a commit. Not yet approving because I want to asses the effort required to automate/enforce this (started playing with something).
I'll defer completion of the enforcement check to a separate PR. (It is pretty clear that this will be necessary, though. For example, right now the Refaster
check doesn't define a URL, and its module (refaster-runner
) does not have access to Documentation.BUG_PATTERNS_BASE_URL
.)
b5dc7e3
to
49c86f4
Compare
No description provided.