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 Collections{Max,Min,Sort} Refaster rules #1297

Merged
merged 2 commits into from
Aug 26, 2024
Merged

Introduce Collections{Max,Min,Sort} Refaster rules #1297

merged 2 commits into from
Aug 26, 2024

Conversation

mohamedsamehsalah
Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah commented Aug 18, 2024 •
edited
Loading

## Built on top of #1276

Suggested commit message:

Introduce `Collections{Max,Min,Sort}` Refaster rules (#1297)

While there, rename `{Max,Min}OfCollection` to
`Collections{Max,Min}WithComparator`.

@mohamedsamehsalah mohamedsamehsalah added this to the 0.19.0 milestone Aug 18, 2024
@mohamedsamehsalah mohamedsamehsalah mentioned this pull request Aug 18, 2024
Merged
@github-actions 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.

mohamedsamehsalah
mohamedsamehsalah commented Aug 18, 2024
View reviewed changes
Copy link
Contributor Author

@mohamedsamehsalah mohamedsamehsalah left a comment

Choose a reason for hiding this comment

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

The following are flagged by default as a warning in Intellij

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
static final class SortCollections<T extends Comparable<? super T>> {
@BeforeTemplate
void before(List<T> collection) {
Collections.sort(collection, naturalOrder());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line.

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java Outdated
@BeforeTemplate
T before(Collection<T> collection) {
return Refaster.anyOf(
Collections.min(collection, naturalOrder()), Collections.max(collection, reverseOrder()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Collections.max(collection, reverseOrder())

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java Outdated
@BeforeTemplate
T before(Collection<T> collection) {
return Refaster.anyOf(
Collections.max(collection, naturalOrder()), Collections.min(collection, reverseOrder()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Collections.min(collection, reverseOrder())

@rickie rickie force-pushed the sschroevers/more-min-max-rules branch from 4982575 to 1ca77b4 Compare August 22, 2024 14:58
Base automatically changed from sschroevers/more-min-max-rules to master August 23, 2024 06:47
@Stephan202 Stephan202 force-pushed the mohamedsamehsalah/collection-min-max branch from 527cf0d to 81d35e6 Compare August 23, 2024 17:12
Stephan202
Stephan202 approved these changes Aug 23, 2024
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.

Rebased and added a commit. Suggested commit message:

Introduce `Collections{Max,Min,Sort}` Refaster rules (#1297)

While there, rename `{Max,Min}OfCollection` to
`Collections{Max,Min}WithComparator`.

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java Outdated
@@ -243,18 +245,77 @@ int after(T value1, T value2) {
}
}

/** Prefer {@link Collections#sort(List)} over more verbose alternatives. */
static final class SortCollections<T extends Comparable<? super T>> {
Copy link
Member

Choose a reason for hiding this comment

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

Following our naming scheme:

Suggested change
static final class SortCollections<T extends Comparable<? super T>> {
static final class SortCollections<T extends Comparable<? super T>> {

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java Outdated
}

/** Prefer {@link Collections#min(Collection)} over more verbose alternatives. */
static final class MinOfCollection<T extends Comparable<? super T>> {
Copy link
Member

Choose a reason for hiding this comment

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

I see how this matches the other rule (should have thought this through better in the other PR 😬), but I realize this one should be named CollectionsMin, and similarly before for the "max" rule. I'll also move it up a bit and rename the MinOfCollectionComparator variant to CollectionsMinWithComparator; the "with" naming is also used elsewhere. (Not sure this'll be the final naming we'll settle on 🤔.)

...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java Outdated
@@ -107,6 +108,24 @@ ImmutableSet<Integer> testCompareTo() {
Comparator.<String>reverseOrder().compare("baz", "qux"));
}

void testSortCollections() {
Collections.sort(List.of("foo", "bar"), naturalOrder());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Collections.sort(List.of("foo", "bar"), naturalOrder());
Collections.sort(ImmutableList.of("foo", "bar"), naturalOrder());

...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java Outdated
Comment on lines 120 to 122
ImmutableSet<String> collection = ImmutableSet.of("foo", "bar");
return ImmutableSet.of(
Collections.min(collection, naturalOrder()), Collections.max(collection, reverseOrder()));
Copy link
Member

Choose a reason for hiding this comment

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

We generally don't create separate variables in cases like these.

@Stephan202 Stephan202 changed the title Introduce {MaxOf,MinOf,Sort}Collection refaster rules Introduce Collections{Max,Min,Sort} Refaster rules Aug 23, 2024
@github-actions 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 August 23, 2024 17:19
@rickie rickie force-pushed the mohamedsamehsalah/collection-min-max branch from 81d35e6 to 7a53dc3 Compare August 26, 2024 05:25
@github-actions 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.

mohamedsamehsalah and others added 2 commits August 26, 2024 07:28
@mohamedsamehsalah @rickie
Introduce {MaxOf,MinOf,Sort}Collection refaster rules
760f06f 
While here, rename original `{MaxOf,MinOf}Collection` to `{MaxOf,MinOf}CollectionComparator`.
@Stephan202 @rickie
Suggestions
ae12270
@rickie rickie force-pushed the mohamedsamehsalah/collection-min-max branch from 7a53dc3 to ae12270 Compare August 26, 2024 05:28
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

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

@rickie rickie merged commit 73ed6e3 into master Aug 26, 2024
16 checks passed
@rickie rickie deleted the mohamedsamehsalah/collection-min-max branch August 26, 2024 05:38
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

@Stephan202 Stephan202 Stephan202 approved these changes

Assignees
No one assigned
Labels
new feature
Milestone
0.19.0
Development

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