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

Introduce SuggestedFixRules Refaster rule collection #559

Merged
merged 4 commits into from
Apr 5, 2023

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Apr 1, 2023

Suggested commit message:

Introduce `SuggestedFixRules` Refaster rule collection (#559)

This PR was written with the aid of GitHub Copilot. It simplifies expressions suggested by Copilot while working on another PR.

@Stephan202 Stephan202 added this to the 0.10.0 milestone Apr 1, 2023
@Stephan202 Stephan202 requested a review from rickie April 1, 2023 10:18
@github-actions
Copy link

github-actions bot commented Apr 1, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie requested a review from werli April 4, 2023 07:40
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Added a commit and added a Refaster rule and some questions. Besides that, LGTM!

static final class SuggestedFixReplaceTree {
@BeforeTemplate
SuggestedFix before(Tree tree, String replaceWith) {
return SuggestedFix.builder().replace(tree, replaceWith).build();
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why the replaceWith part is left out in all titles 👀.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I started out with a name that didn't include it, I think.

}

/** Prefer {@link SuggestedFix#swap(Tree, Tree)} over more contrived alternatives. */
static final class SuggestedFixSwap {
Copy link
Member

Choose a reason for hiding this comment

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

What order do we use here? Should we use the same order as the methods are listed in SuggestedFix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Long-term I guess lexicographical order makes most sense. Here I went with "likelihood of being used".

}

/** Prefer {@link SuggestedFix#swap(Tree, Tree)} over more contrived alternatives. */
static final class SuggestedFixSwap {
Copy link
Member

Choose a reason for hiding this comment

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

What order do we use here? Should we use the same order as the methods are listed in SuggestedFix?

SuggestedFix after(Tree tree, String postfix) {
return SuggestedFix.postfixWith(tree, postfix);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for omitting SuggestedFix#delete(Tree)? I added it :).

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 force-pushed the sschroevers/refaster-suggestedfix branch from cacc84b to e7b47a7 Compare April 4, 2023 08:21
Copy link
Member Author

@Stephan202 Stephan202 left a 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 :)

}

SuggestedFix testSuggestedFixDelete() {
return SuggestedFix.builder().delete((Tree) null).build();
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to cast here. (Another thing we could try to auto-fix.)

static final class SuggestedFixReplaceTree {
@BeforeTemplate
SuggestedFix before(Tree tree, String replaceWith) {
return SuggestedFix.builder().replace(tree, replaceWith).build();
Copy link
Member Author

Choose a reason for hiding this comment

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

Because I started out with a name that didn't include it, I think.

}

/** Prefer {@link SuggestedFix#swap(Tree, Tree)} over more contrived alternatives. */
static final class SuggestedFixSwap {
Copy link
Member Author

Choose a reason for hiding this comment

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

Long-term I guess lexicographical order makes most sense. Here I went with "likelihood of being used".

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Cool stuff 🚀 !

Copy link
Member

@werli werli left a comment

Choose a reason for hiding this comment

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

💅

@rickie
Copy link
Member

rickie commented Apr 5, 2023

Rebased, will merge once 🟢.

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie merged commit 64f9d6b into master Apr 5, 2023
@rickie rickie deleted the sschroevers/refaster-suggestedfix branch April 5, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants