Suggest canonical modifier usage for Refaster template definitions #254
Conversation
| @Override | ||
| public Description matchClass(ClassTree tree, VisitorState state) { | ||
| if (!hasMatchingMember(tree, BEFORE_TEMPLATE_METHOD, state)) { | ||
| /* This is not a Refaster template class. */ |
There was a problem hiding this comment.
| /* This is not a Refaster template class. */ | |
| /* This class is not a Refaster template. */ |
There was a problem hiding this comment.
Hmm, not sure. But will propose something.
| .orElse(Description.NO_MATCH); | ||
| } | ||
|
|
||
| private static SuggestedFix suggestFix(ClassTree tree, VisitorState state) { |
There was a problem hiding this comment.
suggestFix is not ideal IMHO, simply because of the SuggestedFix class. In some other classes we use tryFix..... Maybe we can go for tryFixModifiers? Although clear from context, I think it is a bit clearer about the content of the method.
There was a problem hiding this comment.
suggestCanonicalModifiers maybe?
There was a problem hiding this comment.
"suggest" is present tense, and "suggested" is past tense. So to me it makes at least some sense that a method named suggestFix produces a SuggestedFix. I avoided tryFixModifiers on purpose because I prefer to reserve try for Optional-returning methods (or methods with a conditional side-effect).
But suggestCanonicalModifiers SGTM 👍
| " }", | ||
| "", | ||
| " @BeforeTemplate", | ||
| " // BUG: Diagnostic contains:", |
There was a problem hiding this comment.
Shouldn't beforeStatic come before beforeSynchronized?
There was a problem hiding this comment.
By that logic we should swap more stuff; will add a commit.
|
Added a commit. Suggested commit message looks good. We could go for |
Stephan202
force-pushed
the
sschroevers/refaster-template-modifier-fixes
branch
from
15ecd5e to
841b35e
Compare
| " }", | ||
| "", | ||
| " @BeforeTemplate", | ||
| " // BUG: Diagnostic contains:", |
There was a problem hiding this comment.
By that logic we should swap more stuff; will add a commit.
| @Override | ||
| public Description matchClass(ClassTree tree, VisitorState state) { | ||
| if (!hasMatchingMember(tree, BEFORE_TEMPLATE_METHOD, state)) { | ||
| /* This is not a Refaster template class. */ |
There was a problem hiding this comment.
Hmm, not sure. But will propose something.
| .orElse(Description.NO_MATCH); | ||
| } | ||
|
|
||
| private static SuggestedFix suggestFix(ClassTree tree, VisitorState state) { |
There was a problem hiding this comment.
"suggest" is present tense, and "suggested" is past tense. So to me it makes at least some sense that a method named suggestFix produces a SuggestedFix. I avoided tryFixModifiers on purpose because I prefer to reserve try for Optional-returning methods (or methods with a conditional side-effect).
But suggestCanonicalModifiers SGTM 👍
As discussed here.
Suggested commit message: