Add whitespace check capability and ParenPadQuickFix #298

karlvr · 2021-01-14T08:55:46Z

I've had to introduce a new quick fix type. I've tried to minimise the changes, but perhaps BaseQuickFix should be renamed to BaseASTQuickFix.

(I've also implemented this for the fix all, but I'll add that if / when we merge fix all)

karlvr · 2021-01-14T21:26:45Z

@jdneo I've made a few more improvements to the application of these edit quick fixes, so that we don't apply more than one to a line... because they can conflict a little! I might try to merge those improvements into this PR without adding the additional checks, but I can't PR the additional checks until we land the actual introduction of this different type of quick fix! So if you have a moment to check this out and comment that would make my branch juggling a little easier ;-)

jdneo · 2021-01-15T02:12:46Z

@karlvr Ok, I'll review this PR first. Thanks.

karlvr · 2021-01-15T02:31:32Z

@jdneo thank you; I've just checked. My aforementioned improvements to the application of these edit quick fixes actually only applies (of course) to the "fix all" support. Without meaning to be super cheeky, once / if you're happy with the "fix all" then I'll land that piece! This one is good to go as is 🤞

jdneo · 2021-01-15T02:30:15Z

...gchen.checkstyle.runner/src/main/java/com/shengchen/checkstyle/quickfix/QuickFixService.java

+            astRoot.accept(((BaseQuickFix) quickFix).getCorrectingASTVisitor(lineInfo, offset.intValue()));
+            final TextEdit edit = astRoot.rewrite(document, null);
+            return EditUtils.convertToWorkspaceEdit(unit, edit);
+        } else if (quickFix instanceof BaseEditQuickFix) {


For simplicity, how about write the if block as

if (quickFix instanceof BaseQuickFix) { ... } else if (quickFix instanceof BaseEditQuickFix) { final TextEdit edit = ((BaseEditQuickFix) quickFix).createTextEdit(lineInfo, offset.intValue(), document); if (edit != null) { return EditUtils.convertToWorkspaceEdit(unit, edit); } } return null;

jdneo · 2021-01-15T02:41:29Z

From the implementation, it looks like we are deleting spaces in the parentheses. For example: switch( n ) -> switch(n)

But according to the document: https://checkstyle.sourceforge.io/property_types.html#PadOption

The PadOption can be nospace,space, which means whether adding/deleting the spaces actually relies on how the rule is configured.

@karlvr I'm afraid that simply removing the spaces is not enough for this fix.

karlvr · 2021-01-15T03:10:43Z

@jdneo yes! I would love to work out how to determine what the option to the check was! It doesn't appear that Checker provides a way to interrogate the checks it uses. Do you know of a way to do that? Alternatively we could perhaps inject the diagnostic message as well, and work out what to do based on the message to the user?

(Who are these people adding spaces inside the parentheses?!)__

karlvr · 2021-01-19T18:32:06Z

I'm trying to figure out how to read the Checkstyle configuration to solve the above...

karlvr · 2021-01-20T20:58:41Z

@jdneo I haven't yet looked into how to read the Checkstyle configuration, but I have pushed changes to allow these edits to work in the "fix all" mode, and added a couple more whitespace-related quick fixes.

karlvr · 2021-01-24T18:54:37Z

@jdneo It looks like we could use ConfigurationLoader.loadConfiguration to load the Checkstyle DefaultConfiguration and then explore it, looking for how things are configured in order to resolve #298 (comment) but I don't think that will work generally... there could be multiple configurations for a check, matching different files... I think it might get messy.

I'm exploring examining the diagnostic message to determine how to fix it. In the Checkstyle source I see this documentation on LocalizedMessage.getKey():

Returns the message key to locate the translation, can also be used in IDE plugins to map audit event messages to corrective actions.

So I think this is the right track.

This will provide more information to the quick fixes about exactly what the violation was, so the quick fix can respond to the configuration.

karlvr · 2021-01-25T09:48:32Z

@jdneo I have resolved the issue that you raised about regarding PadOption in the ParenPadQuickFix.

I've noticed an issue, however, when an AST fix and a whitespace fix coincide:

public static void main(String[] args) {

becomes

public static void main(final  String[] args ) {

when ParenPadCheck is configured with space and FinalParametersCheck is included.

<module name="TreeWalker">
	<module name="FinalParameters" />
	<module name="ParenPad">
		<property name="option" value="space" />
	</module>
</module>

e.g. public static void main(String[] args) { becomes public static void main(final String[] args ) { under <module name="TreeWalker"> <module name="FinalParameters" /> <module name="ParenPad"> <property name="option" value="space" /> </module> </module>

karlvr · 2021-01-25T10:44:52Z

@jdneo OK, there are a bunch of whitespace-related quick fixes in here. It taking implementing a few to end up with a reasonable pattern I think. Please take it for a spin where you get a chance.

jdneo · 2021-01-29T05:49:51Z

...gchen.checkstyle.runner/src/main/java/com/shengchen/checkstyle/quickfix/QuickFixService.java

-                final IRegion lineInfo = document.getLineInformationOfOffset(offset);
-                astRoot.accept(quickFix.getCorrectingASTVisitor(lineInfo, offset));
+            final IRegion lineInfo = document.getLineInformationOfOffset(offset);
+            final IQuickFix quickFix = getQuickFix(sourceNames.get(i));


resolve the quickFix at first and continue the loop if it's null to reduce unnecessary calculation?

jdneo · 2021-01-29T05:52:56Z

...ner/src/main/java/com/shengchen/checkstyle/quickfix/whitepace/GenericWhitespaceQuickFix.java

+
+            if ("ws.illegalFollow".equals(violationKey)) {
+                return new InsertEdit(markerStartOffset + 1, " ");
+            } else if ("ws.notPreceded".equals(violationKey)) {


It would be better if we can directly reference the static const defined in the Checkstyle lib, seems it's not easily achievable given the current OSGi framework. 🤔

@jdneo yes, I agree. I don't have experience with OSGi or how all of those dependencies are setup so I took the path of least resistance.

jdneo · 2021-01-29T06:14:30Z

...runner/src/main/java/com/shengchen/checkstyle/quickfix/whitepace/MethodParamPadQuickFix.java

+ * Quick fix for
+ * https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/checks/whitespace/MethodParamPadCheck.html
+ */
+public class MethodParamPadQuickFix extends BaseEditQuickFix {


Seems not handle well with:

<module name="MethodParamPad"> <property name="tokens" value="METHOD_DEF"/> <property name="option" value="space"/> <property name="allowLineBreaks" value="true"/> </module>

@jdneo when I test this, Checkstyle reports that the "(" is not preceded by whitespace, and the quick fix adds a space before the "(". That seems like it's doing the right thing to me?

jdneo · 2021-01-29T06:16:30Z

...er/src/main/java/com/shengchen/checkstyle/quickfix/whitepace/NewlineAtEndOfFileQuickFix.java

+            final String lastChar = doc.getLength() > 0 ? doc.get(doc.getLength() - 1, 1) : "";
+
+            if (lastChar.isEmpty() || !"\n".equals(lastChar)) {
+                return new InsertEdit(doc.getLength(), "\n");


\n might not be enough. Given this rule has the options:

<property name="lineSeparator" value="lf"/>

@jdneo hmm, yes; given the error that is reported doesn't specify what it's looking for, I think the best we can do is assume the default (which looks for any of CR LF or CRLF) and then adds the platform-specific version.

Or we could simply add the platform specific version.

Is it handy enough to fix these things that it might be wrong sometimes? Or should we more actively pursue getting Checkstyle to expose its internal configuration? My forays into the code suggest that it doesn't want to.

karlvr · 2021-03-02T23:43:09Z

@jdneo just a ping to see if you have bandwidth to pick this back up?

jdneo · 2021-03-03T01:49:55Z

@karlvr Sorry I was busy with some other stuffs, will try to take a look some time by the end of this week.

jdneo reviewed Jan 15, 2021

View reviewed changes

jdneo added the enhancement label Jan 15, 2021

Add whitespace check capability and ParenPadQuickFix

348c685

karlvr force-pushed the quickfix-whitespace branch from 2369648 to 348c685 Compare January 20, 2021 05:50

karlvr added 4 commits January 20, 2021 18:57

Fix whitespace quick fix compatibility with AST fixes

32aa9ea

Add WhitespaceAfter quick fix

380573a

Add WhitespaceAround quick fix

156dc4d

Fix applying edit quick fixes with AST quick fixes

f2b6bdd

karlvr added 4 commits January 21, 2021 15:09

Add NoWhitespaceBeforeCheck quick fix

a345f72

Fix WhitespaceAroundQuickFix with non-blocks

33b391c

Add NewlineAtEndOfFileQuickFix

4dcae0a

Add NoWhitespaceAfterCheck quick fix

8de767e

karlvr added 3 commits January 25, 2021 22:41

Support Checkstyle violation message key

85ae4db

This will provide more information to the quick fixes about exactly what the violation was, so the quick fix can respond to the configuration.

Pass Checkstyle violation key to text edit quick fixes

7bcf764

ParenPadQuickFix: use violation key to choose correct fix

cd2444b

karlvr added 5 commits January 25, 2021 22:59

Move whitespace utility functions into BaseEditQuickFix

5843bf6

Add GenericWhitespaceQuickFix

9de7299

Add MethodParamPadQuickFix

bfe328b

Merge remote-tracking branch 'origin/master' into quickfix-whitespace

dc0d9ee

Merge branch 'master' into quickfix-whitespace

1c3d306

jdneo reviewed Jan 29, 2021

View reviewed changes

karlvr added 2 commits February 9, 2021 19:07

Check whether a quickfix exists early

3527444

NewlineAtEndOfFileQuickFix: match defaults of NewlineAtEndOfFileCheck

5ba919b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add whitespace check capability and ParenPadQuickFix #298

Add whitespace check capability and ParenPadQuickFix #298

karlvr commented Jan 14, 2021

karlvr commented Jan 14, 2021

jdneo commented Jan 15, 2021

karlvr commented Jan 15, 2021

jdneo Jan 15, 2021

jdneo commented Jan 15, 2021 •

edited

Loading

karlvr commented Jan 15, 2021

karlvr commented Jan 19, 2021

karlvr commented Jan 20, 2021

karlvr commented Jan 24, 2021

karlvr commented Jan 25, 2021 •

edited

Loading

karlvr commented Jan 25, 2021

jdneo Jan 29, 2021

jdneo Jan 29, 2021

karlvr Feb 9, 2021

jdneo Jan 29, 2021

karlvr Feb 9, 2021

jdneo Jan 29, 2021

karlvr Feb 9, 2021

karlvr commented Mar 2, 2021

jdneo commented Mar 3, 2021

Add whitespace check capability and ParenPadQuickFix #298

Are you sure you want to change the base?

Add whitespace check capability and ParenPadQuickFix #298

Conversation

karlvr commented Jan 14, 2021

karlvr commented Jan 14, 2021

jdneo commented Jan 15, 2021

karlvr commented Jan 15, 2021

Choose a reason for hiding this comment

jdneo commented Jan 15, 2021 • edited Loading

karlvr commented Jan 15, 2021

karlvr commented Jan 19, 2021

karlvr commented Jan 20, 2021

karlvr commented Jan 24, 2021

karlvr commented Jan 25, 2021 • edited Loading

karlvr commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlvr commented Mar 2, 2021

jdneo commented Mar 3, 2021

jdneo commented Jan 15, 2021 •

edited

Loading

karlvr commented Jan 25, 2021 •

edited

Loading