Add setting for clean ups to be applied upon document save #2298

datho7561 · 2022-10-27T16:01:16Z

Add new setting java.cleanup.actionsOnSave to list which clean ups to run
Add clean up to prefix all member accesses with this
Add clean up to prefix all static member accesses with class name
Add clean up to insert missing @Override
Add clean up to insert missing @Deprecated

Signed-off-by: David Thompson davthomp@redhat.com

eclipse-ls-bot · 2022-10-27T16:01:20Z

Can one of the admins verify this patch?

rgrunber · 2022-10-27T19:44:46Z

So all we have to do on the client side is set java.cleanup.enabled to contain the list of cleanup identifiers ? Have some good defaults and it will allow use to build from that.

I think I like this approach. It's basically what editor.codeActionsOnSave does in VS Code, but this is generalized so all clients could take advantage.

datho7561 · 2022-10-27T20:10:42Z

Some things I've run into:

some of the more advanced clean ups have more advanced configuration, so they might not be able to be configured as neatly
some of the clean ups rely on using the error markers. For instance, the add @Deprecated clean up requires enabling a compiler setting in the (JDT) project settings in order to work

- Add documentation (accessible through a command) for what each clean up does Depends on eclipse-jdtls/eclipse.jdt.ls#2298 Closes redhat-developer#2144 Signed-off-by: David Thompson <davthomp@redhat.com>

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/Preferences.java

...jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/AddDeprecatedAnnotationCleanUp.java

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/INoConfigCleanUp.java

rgrunber · 2022-11-02T19:30:01Z

How difficult would it be to set the corresponding problem to warning/error that needs to be enabled in order to permit the cleanup to activate ? I'm considering either that or we just stick to cleanups that don't rely on error markers at all.

datho7561 · 2022-11-03T12:52:48Z

How difficult would it be to set the corresponding problem to warning/error that needs to be enabled in order to permit the cleanup to activate ?

I tried to get it to enable the required problems for just when applying the clean up, so that we don't need to change any of the project configuration on disk, but I got stuck. I can try again.

I could also look into enabling those warnings when importing a non Eclipse project if that's a good enough solution.

- Add documentation (accessible through a command) for what each clean up does Depends on eclipse-jdtls/eclipse.jdt.ls#2298 Closes redhat-developer#2144 Signed-off-by: David Thompson <davthomp@redhat.com>

datho7561 · 2022-11-03T19:54:30Z

I'm in the process of making a change where JDT.LS will update the preferences before applying a cleanup that requires a compiler warning to be enabled, but have also found a few bugs in the process. Specifically:

If you list a cleanup multiple times it will be applied multiple times (which I knew because:)
Each clean up (after the first) doesn't take into account that the changes that the previous clean up made

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/CleanUpUtils.java

rgrunber · 2022-11-08T14:40:07Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/CleanUpRegistry.java

+		}
+
+		// build the context after setting the compiler options so that the built AST has all the required markers
+		CleanUpContextCore context = CleanUpUtils.getCleanUpContext(textDocumentId, opts, monitor);


If possible, we should only get to this stage (parsing the AST), if we know there's cleanups to apply. So if the list is empty, maybe we exit earlier (or return empty ? )

Good catch. I'll fix this soon. I also ran into an issue with how we were handling spaces vs tabs that I'll address

I've gotten no where with the spaces vs tabs issue I ran into so I'll push my fix for this.

datho7561 · 2022-11-08T20:09:57Z

Sometimes when I save, spaces are used instead of tabs in the clean ups, despite setting up an .editorconfig to use tabs and setting:

"[java]": {
    "editor.insertSpaces": false,
    "editor.tabSize": 4
  },

in the vscode settings

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/CleanUpRegistry.java

- Add documentation (accessible through a command) for what each clean up does Depends on eclipse-jdtls/eclipse.jdt.ls#2298 Closes redhat-developer#2144 Signed-off-by: David Thompson <davthomp@redhat.com>

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/CleanUpUtils.java

- Add documentation (accessible through a command) for what each clean up does Depends on eclipse-jdtls/eclipse.jdt.ls#2298 Closes redhat-developer#2144 Signed-off-by: David Thompson <davthomp@redhat.com>

- Add new setting `java.cleanups.enabled` to list which clean ups to run - Add clean up to prefix all member accesses with `this` - Add clean up to prefix all static member accesses with class name - Add clean up to insert missing `@Override` - Add clean up to insert missing `@Deprecated` See redhat-developer/vscode-java#2144 Signed-off-by: David Thompson <davthomp@redhat.com>

rgrunber

Change looks good to me. The setting defaults to empty for now but perhaps in the future we can impose some nice defaults.

Maybe in the future we even automatically apply some based on the detected project source level.

rgrunber · 2022-11-09T18:07:13Z

ok to test.

rgrunber · 2022-11-09T18:34:29Z

test this please.

- Add documentation (accessible through a command) for what each clean up does Depends on eclipse-jdtls/eclipse.jdt.ls#2298 Closes redhat-developer#2144 Signed-off-by: David Thompson <davthomp@redhat.com>

- Add documentation (accessible through a command) for what each clean up does Depends on eclipse-jdtls/eclipse.jdt.ls#2298 Closes #2144 Signed-off-by: David Thompson <davthomp@redhat.com>

datho7561 force-pushed the 2144-generate-override branch from d15eab9 to a69f1b5 Compare October 27, 2022 18:00

datho7561 force-pushed the 2144-generate-override branch 2 times, most recently from a56a3a0 to 24643c2 Compare October 27, 2022 20:05

datho7561 force-pushed the 2144-generate-override branch from 24643c2 to d88c764 Compare October 28, 2022 12:13

datho7561 mentioned this pull request Oct 28, 2022

Add new settings for clean ups redhat-developer/vscode-java#2768

Merged

datho7561 force-pushed the 2144-generate-override branch from d88c764 to b91e9d8 Compare October 28, 2022 15:51

datho7561 changed the title ~~Add clean ups (configurable fixes that are run on save)~~ Add clean ups (fixes that are run on save) Oct 28, 2022

datho7561 force-pushed the 2144-generate-override branch from b91e9d8 to 94cfb43 Compare October 28, 2022 18:12

datho7561 marked this pull request as ready for review October 28, 2022 18:41

rgrunber reviewed Nov 2, 2022

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/Preferences.java Outdated Show resolved Hide resolved

rgrunber reviewed Nov 2, 2022

View reviewed changes

...jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/AddDeprecatedAnnotationCleanUp.java Outdated Show resolved Hide resolved

rgrunber reviewed Nov 2, 2022

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/INoConfigCleanUp.java Outdated Show resolved Hide resolved

datho7561 force-pushed the 2144-generate-override branch 2 times, most recently from 86d9614 to 5345c10 Compare November 3, 2022 14:41

datho7561 force-pushed the 2144-generate-override branch from 5345c10 to b2d7e3a Compare November 3, 2022 16:14

datho7561 force-pushed the 2144-generate-override branch from b2d7e3a to 083f738 Compare November 4, 2022 20:03

rgrunber reviewed Nov 7, 2022

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/CleanUpUtils.java Outdated Show resolved Hide resolved

datho7561 force-pushed the 2144-generate-override branch from 083f738 to 90d1352 Compare November 7, 2022 16:20

datho7561 force-pushed the 2144-generate-override branch from 90d1352 to 1df018f Compare November 7, 2022 18:26

rgrunber reviewed Nov 8, 2022

View reviewed changes

datho7561 force-pushed the 2144-generate-override branch from 1df018f to a62bb8a Compare November 8, 2022 20:07

rgrunber reviewed Nov 8, 2022

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/CleanUpRegistry.java Outdated Show resolved Hide resolved

datho7561 force-pushed the 2144-generate-override branch from a62bb8a to f917d15 Compare November 8, 2022 20:19

rgrunber reviewed Nov 8, 2022

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/cleanup/CleanUpUtils.java Outdated Show resolved Hide resolved

datho7561 force-pushed the 2144-generate-override branch 2 times, most recently from 3a6630f to 842676d Compare November 8, 2022 21:55

datho7561 force-pushed the 2144-generate-override branch from 842676d to 66b4f4e Compare November 9, 2022 15:19

rgrunber changed the title ~~Add clean ups (fixes that are run on save)~~ Add setting for clean ups to be applied upon document save Nov 9, 2022

rgrunber added the enhancement label Nov 9, 2022

rgrunber added this to the End November milestone Nov 9, 2022

rgrunber approved these changes Nov 9, 2022

View reviewed changes

rgrunber merged commit c9a2d05 into eclipse-jdtls:master Nov 9, 2022

datho7561 deleted the 2144-generate-override branch November 9, 2022 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting for clean ups to be applied upon document save #2298

Add setting for clean ups to be applied upon document save #2298

datho7561 commented Oct 27, 2022 •

edited

Loading

eclipse-ls-bot commented Oct 27, 2022

rgrunber commented Oct 27, 2022 •

edited

Loading

datho7561 commented Oct 27, 2022

rgrunber commented Nov 2, 2022

datho7561 commented Nov 3, 2022

datho7561 commented Nov 3, 2022 •

edited

Loading

rgrunber Nov 8, 2022

datho7561 Nov 8, 2022

datho7561 Nov 8, 2022

datho7561 commented Nov 8, 2022

rgrunber left a comment

rgrunber commented Nov 9, 2022

rgrunber commented Nov 9, 2022

Add setting for clean ups to be applied upon document save #2298

Add setting for clean ups to be applied upon document save #2298

Conversation

datho7561 commented Oct 27, 2022 • edited Loading

eclipse-ls-bot commented Oct 27, 2022

rgrunber commented Oct 27, 2022 • edited Loading

datho7561 commented Oct 27, 2022

rgrunber commented Nov 2, 2022

datho7561 commented Nov 3, 2022

datho7561 commented Nov 3, 2022 • edited Loading

rgrunber Nov 8, 2022

Choose a reason for hiding this comment

datho7561 Nov 8, 2022

Choose a reason for hiding this comment

datho7561 Nov 8, 2022

Choose a reason for hiding this comment

datho7561 commented Nov 8, 2022

rgrunber left a comment

Choose a reason for hiding this comment

rgrunber commented Nov 9, 2022

rgrunber commented Nov 9, 2022

datho7561 commented Oct 27, 2022 •

edited

Loading

rgrunber commented Oct 27, 2022 •

edited

Loading

datho7561 commented Nov 3, 2022 •

edited

Loading