-
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
Improve Tree
deletion suggestions
#347
Improve Tree
deletion suggestions
#347
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.
Rebased and added a commit.
WDYT about removing the XXX
at the top of the class of SourceCode
? I think by now it kinda "fits" in the util
package together with the other utilities.
Nice and creative way of testing this 🚀 !
} | ||
|
||
/** | ||
* Uses {@link SourceCode#deleteWithTrailingWhitespace(Tree, VisitorState)} to suggest the |
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 all @BugPattern
s we start with A {@link BugChecker} that
, so would suggest to do the same here.
fcf3521
to
defbb27
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.
I don't see a second commit 👀
WDYT about removing the
XXX
at the top of the class ofSourceCode
? I think by now it kinda "fits" in theutil
package together with the other utilities.
Works for me!
Rebased and added a commit ;)
Oops, I don't know what happened but thanks for applying 😉. |
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'll be afk for the next >24 hours; can add a commit later.
int actualEnd = NON_WHITESPACE_MATCHER.indexIn(sourceCode, endPos); | ||
return actualEnd == -1 | ||
? SuggestedFix.delete(tree) | ||
: SuggestedFix.replace(((DiagnosticPosition) tree).getStartPosition(), actualEnd, ""); |
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.
Not that I think we ever hit this case, but I think we should use sourceCode.length()
if actualEnd == -1
.
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.
When suggesting to remove a method or method annotation, also remove any trailing whitespace. This avoids the possible introduction of an empty line right at the start of a code block.
defbb27
to
bdb60ee
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.
Looks good 🚀
Suggested commit message:
See also this comment.