Skip to content
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

Create cleanup actions for some existing quick assists. #2350

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

rgrunber
Copy link
Contributor

@rgrunber rgrunber commented Nov 25, 2022

  • Add final modifier where possible
  • Convert switch statement to switch expression
  • Use pattern matching for instanceof checks
  • Convert anonymous function to lambda expression
  • Add test cases, and set CleanUpsTest compliance to 19

Signed-off-by: Roland Grunberg [email protected]

@datho7561
Copy link
Contributor

If I use

package org.acme;

import java.io.File;
import java.io.FileFilter;
import java.util.Arrays;

public class RolandsTest {
    public void test() {
        String PATH = "/this/is/some/path";
        String MESSAGE = """
                This is a message.\
                This message has multiple lines.\
                We can convert it to a text block""";

        final Object[] obj = Arrays.asList(PATH).toArray();
        if (obj[0] instanceof String) {
            String tmp = (String) obj[0];
            File f = new File(tmp);
            File[] filtered = f.listFiles(new FileFilter() {
                @Override
                public boolean accept(File path) {
                    return true;
                }
            });
        }
    }
};

with

"java.cleanup.actionsOnSave": [
  "addOverride",
  "stringConcatToTextBlock",
  "invertEquals",
  "addFinalModifier",
  "lambdaExpression",
],

I get an NPE

@rgrunber
Copy link
Contributor Author

rgrunber commented Nov 30, 2022

Thanks. It's reproducible in the tests as well. I forgot I changed the API of the cleanup retrieval to return null instead of an empty list when moving from LSP4J TextEdit -> Eclipse TextEdit.

Also need to update the original tests to compare content.

@rgrunber rgrunber force-pushed the easy-cleanups branch 2 times, most recently from 1e87984 to 1edeb06 Compare December 2, 2022 20:18
@rgrunber rgrunber marked this pull request as ready for review December 2, 2022 20:53
@rgrunber
Copy link
Contributor Author

rgrunber commented Dec 2, 2022

Also, if we ever need to implement a proper TextEdit for the cleanups, we could use https:/google/diff-match-patch/blob/master/java/src/name/fraser/neil/plaintext/diff_match_patch.java#L1769 .

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works very well!

- Add final modifier where possible
- Convert switch statement to switch expression
- Use pattern matching for instanceof checks
- Convert anonymous class creation to lambda expression
- Add test cases, and set CleanUpsTest compliance to 19
- Adjust existing tests to make assertions based on content
- Due to LSP limitations (no overlapping text edits), we now return the
  final state of the document after all cleanups are applied as a text
  edit

Signed-off-by: Roland Grunberg <[email protected]>
Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks, Roland!

@rgrunber rgrunber added this to the Early January 2023 milestone Dec 5, 2022
@rgrunber rgrunber merged commit 73fb18c into eclipse-jdtls:master Dec 7, 2022
@rgrunber rgrunber deleted the easy-cleanups branch December 7, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants