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

Extend StreamRules Refaster rule collection #605

Merged
merged 4 commits into from
May 4, 2023
Merged

Extend StreamRules Refaster rule collection #605

merged 4 commits into from
May 4, 2023

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Apr 29, 2023 •
edited
Loading

Suggested commit message:

Extend `StreamRules` Refaster rule collection (#605)

All changes suggested by SonarCloud's `java:s4034` rule, as well as the
examples mentioned in openrewrite/rewrite#2984 are now covered. (In a
number of cases through composition of more generic rules.)

See https://rules.sonarsource.com/java/RSPEC-4034

@Stephan202 Stephan202 added this to the 0.10.0 milestone Apr 29, 2023
@github-actions
Copy link

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/rspec-4034 branch from 6df4877 to 360db11 Compare April 29, 2023 15:45
@github-actions
Copy link

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 requested a review from rickie April 29, 2023 15:49
@Stephan202 Stephan202 mentioned this pull request Apr 29, 2023
Open
rickie
rickie approved these changes May 2, 2023
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.

Changes LGTM!

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java
@@ -247,12 +248,13 @@ Optional<S> after(Stream<T> stream, Function<? super T, S> function) {
/** In order to test whether a stream has any element, simply try to find one. */
static final class StreamIsEmpty<T> {
@BeforeTemplate
boolean before(Stream<T> stream) {
boolean before(Stream<T> stream, Collector<? super T, ?, ? extends Collection<?>> collector) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it nicer to have a separate before template for this one?

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 strong opinion. (Something we could enforce one way or another with automation...)

Venorcis
Venorcis reviewed May 3, 2023
View reviewed changes
error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java
return Refaster.anyOf(
stream.count() == 0,
stream.count() <= 0,
stream.count() < 1,
stream.findFirst().isEmpty());
stream.findFirst().isEmpty(),
stream.collect(collector).isEmpty());
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't you theoretically use a filtering collector though?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! We should add a comment calling out this caveat. I'll add a commit later this morning 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Added two more comments. Since this kind of is rare (and since we rewrite top-level filtering collectors to a regular filter step), I think this is an acceptable limitation for now.

@Stephan202 Stephan202 force-pushed the sschroevers/rspec-4034 branch from 360db11 to db9d36e Compare May 3, 2023 08:35
@github-actions
Copy link

github-actions bot commented May 3, 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.

2 similar comments
@github-actions
Copy link

github-actions bot commented May 3, 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.

@github-actions
Copy link

github-actions bot commented May 3, 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 force-pushed the sschroevers/rspec-4034 branch from ebf5131 to 997eb0f Compare May 3, 2023 12:49
@github-actions
Copy link

github-actions bot commented May 3, 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
Copy link
Member Author

👍

Stephan202 and others added 4 commits May 3, 2023 16:56
@Stephan202 @rickie
Extend StreamRules Refaster rule collection
37989ec 
All changes suggested by SonarCloud's `java:s4034` rule, as well as the
examples mentioned in openrewrite/rewrite#2984 are now covered. (In a
number of cases through composition of more generic rules.)

See https://rules.sonarsource.com/java/RSPEC-4034
@Stephan202 @rickie
Add a comment
976725a
@Stephan202 @rickie
One more idea
e146b5e
@rickie
Drop this
70b4554
@rickie rickie force-pushed the sschroevers/rspec-4034 branch from 997eb0f to 70b4554 Compare May 3, 2023 14:56
@github-actions
Copy link

github-actions bot commented May 3, 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.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rickie rickie merged commit 6229fa9 into master May 4, 2023
@rickie rickie deleted the sschroevers/rspec-4034 branch May 4, 2023 06:08
philleonard pushed a commit that referenced this pull request May 26, 2023
@Stephan202 @philleonard
Extend StreamRules Refaster rule collection (#605)
bfb3c77 
All changes suggested by SonarCloud's `java:s4034` rule, as well as the
examples mentioned in openrewrite/rewrite#2984 are now covered. (In a
number of cases through composition of more generic rules.)

See https://rules.sonarsource.com/java/RSPEC-4034
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

@Venorcis Venorcis Venorcis approved these changes

Assignees
No one assigned
Labels
new feature
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Stephan202 @Venorcis @rickie