Open karlvr opened 3 years ago
@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 ;-)
@karlvr Ok, I'll review this PR first. Thanks.
@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 🤞
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.
@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?!)__
I'm trying to figure out how to read the Checkstyle configuration to solve the above...
@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.
@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 https://github.com/jdneo/vscode-checkstyle/pull/298#issuecomment-760607226 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.
@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>
@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 just a ping to see if you have bandwidth to pick this back up?
@karlvr Sorry I was busy with some other stuffs, will try to take a look some time by the end of this week.
I've had to introduce a new quick fix type. I've tried to minimise the changes, but perhaps
BaseQuickFix
should be renamed toBaseASTQuickFix
.(I've also implemented this for the fix all, but I'll add that if / when we merge fix all)