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.

Sign up for GitHub

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

Jump to bottom

Introduce CollectionForEach Refaster rule #390

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Introduce CollectionForEach Refaster rule #390

merged 2 commits into from
Dec 6, 2022

Conversation

amestoyg
Copy link
Contributor

@amestoyg amestoyg commented Dec 5, 2022

Related to the issue #387, I added a new Refaster rule to rewrite the use of forEach when working with collections.

@amestoyg amestoyg requested review from Stephan202 and rickie December 5, 2022 23:02
Stephan202
Stephan202 reviewed Dec 6, 2022
View reviewed changes
Copy link
Member

@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.

Nice; left two suggestions. Congrats on your first contribution! 🎉

NB: The build fails because of actions/setup-java#422; if that issue doesn't resolve itself soon we'll open a downgrade PR for this repo.

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java Outdated Show resolved Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java Outdated Show resolved Hide resolved
@Stephan202 Stephan202 added this to the 0.6.0 milestone Dec 6, 2022
werli
werli approved these changes Dec 6, 2022
View reviewed changes
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.

Optimistically approving assuming @Stephan202's suggestions will be taken care of. 🙂

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

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.

@amestoyg
Copy link
Contributor Author

amestoyg commented Dec 6, 2022

Thanks for the comments @Stephan202. I pushed the suggested changes 🙂

@rickie rickie changed the title Introduce CollectionForEach Refaster rule Introduce CollectionForEach Refaster rule Dec 6, 2022
@Stephan202
Copy link
Member

Stephan202 commented Dec 6, 2022 •
edited by rickie
Loading

Nice! Suggested commit message:

Introduce `CollectionForEach` Refaster rule (#390)

Fixes #387.

@rickie rickie linked an issue Dec 6, 2022 that may be closed by this pull request
Rewrite collection.stream().forEach(consumer) to collection.forEach(consumer) #387
Closed
2 tasks
rickie
rickie approved these changes Dec 6, 2022
View reviewed changes
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.

Nice @amestoyg, congrats on your first PR 🎉!

@Stephan202 tweaked suggested commit message.
I linked the PR already, but still I think we should have the habit to refer to the tickets in the commit message.

Do we either prefer the terminology "Fixes", "Resolves" or "Closes"?

@rickie rickie force-pushed the amestoyg/collection-foreach branch from a892d61 to 67b4b9a Compare December 6, 2022 08:21
@rickie
Copy link
Member

rickie commented Dec 6, 2022 •
edited
Loading

Rebased.

Do you also approve @Stephan202 😉?

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

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 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@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.

Doh. Of course! 🎉

@rickie rickie merged commit a6f794d into PicnicSupermarket:master Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickie rickie rickie approved these changes

@werli werli werli approved these changes

@Stephan202 Stephan202 Stephan202 approved these changes

Assignees
No one assigned
Labels
new feature
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.

Rewrite collection.stream().forEach(consumer) to collection.forEach(consumer)
4 participants
@amestoyg @Stephan202 @rickie @werli