Introduce `Sets{Difference,Intersection}{,Map,Multimap}` and `SetsUnion` Refaster rules #607

mohamedsamehsalah · 2023-05-01T05:41:10Z

~~Original discussion.~~

Suggested commit message:

Introduce `Sets{Difference,Intersection}{,Map,Multimap}` and `SetsUnion` Refaster rules (#607)

github-actions · 2023-05-01T05:43:16Z

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

Nice improvements @mohamedsamehsalah! 🚀

They make things more clear 😄.

Added a commit and rebased.

Suggested commit message:

Introduce `Sets{Difference,Intersection}{,Map,Multimap}` and `SetsUnion` Refaster rules (#607)

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java

...trib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java

rickie · 2023-05-02T07:13:58Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java

@@ -211,4 +214,43 @@ ImmutableSet<T> after(T e1, T e2, T e3, T e4, T e5) {
      return ImmutableSet.of(e1, e2, e3, e4, e5);
    }
  }
+
+  /** Prefer {@link Sets#intersection(Set, Set)} ()} over more contrived alternatives. */
+  static final class SetsIntersection<T> {


To be honest, it's a bit hard to decide how we should name these rules. We invoke the same method in the three rules but how we get the set that we pass differs. Till now most cases also use different overloads of a method and based on that we use different words.

I imagine we'd also have something that determines a suffix based on what we pass as the two parameters for Sets#intersection.

Was doubting whether we want to add the ImmutableCopy as suffix as well 🤔.

Pushed a suggestion, let's discuss 👀.

I think that in the end we will want to add ImmutableCopy (a.o), but fine for me to merge as-is for now.

github-actions · 2023-05-02T07:21:55Z

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

Added a commit. Did not yet look at union; will do another pass later to see whether that's a reasonable addition to this PR.

Nice idea @mohamedsamehsalah!

Stephan202 · 2023-05-02T09:52:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java

@@ -211,4 +214,43 @@ ImmutableSet<T> after(T e1, T e2, T e3, T e4, T e5) {
      return ImmutableSet.of(e1, e2, e3, e4, e5);
    }
  }
+
+  /** Prefer {@link Sets#intersection(Set, Set)} ()} over more contrived alternatives. */
+  static final class SetsIntersection<T> {


I think that in the end we will want to add ImmutableCopy (a.o), but fine for me to merge as-is for now.

...trib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java

Stephan202 · 2023-05-02T09:53:24Z

...trib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetRulesTestInput.java

+  }
+
+  ImmutableSet<Integer> testSetsIntersectionMultimap() {
+    ImmutableMultimap<Integer, Integer> multimap = ImmutableMultimap.of(1, 2);


It's only a test, but let's avoid the ImmutableMultimap type (ImmutableListMultimap and ImmutableSetMultimap should be preferred.)

What is the reason behind this preference ?

Check the documentation :)

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java

Stephan202 · 2023-05-02T10:12:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetRules.java

+
+    @AfterTemplate
+    ImmutableSet<T> after(Set<T> set1, Set<T> set2) {
+      return Sets.intersection(set1, set2).immutableCopy();


Next to intersection there's also difference. (And union; see the internal Store PR I filed a few days ago.)

~~For reference: #2545~~

@mohamedsamehsalah we try not to link to private things in this repo; I left it out on purpose. 🙃

github-actions · 2023-05-02T10:16:41Z

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 · 2023-05-02T10:38:33Z

@Stephan202
...Nice idea @mohamedsamehsalah!

Original idea from @werli 🚀

Stephan202

Added one more commit :)

github-actions · 2023-05-02T10:55:22Z

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.

werli

Nice, thanks @mohamedsamehsalah! Also thanks to the other reviewers. I got even more than I suggested back then. 😉

github-actions · 2023-05-03T13:35:16Z

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 · 2023-05-03T13:35:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

…on` Refaster rules (#607)

mohamedsamehsalah requested review from Stephan202, werli and rickie May 1, 2023 05:41

rickie force-pushed the mohamedsamehsalah/sets-intersection branch from 5b58f40 to b653978 Compare May 2, 2023 07:19

rickie approved these changes May 2, 2023

View reviewed changes

rickie added this to the 0.10.0 milestone May 2, 2023

rickie added the new feature label May 2, 2023

Stephan202 reviewed May 2, 2023

View reviewed changes

Stephan202 approved these changes May 2, 2023

View reviewed changes

werli approved these changes May 3, 2023

View reviewed changes

mohamedsamehsalah and others added 4 commits May 3, 2023 15:33

Prefer Sets#intersection over more contrived alternatives.

3c162cb

Suggestions

3f096fa

Suggestions

704e38d

Suggestion

215622d

rickie force-pushed the mohamedsamehsalah/sets-intersection branch from 5b65230 to 215622d Compare May 3, 2023 13:33

rickie merged commit 7d728e9 into master May 3, 2023

rickie deleted the mohamedsamehsalah/sets-intersection branch May 3, 2023 14:56

rickie changed the title ~~Prefer Sets#intersection over more contrived alternatives.~~ Introduce Sets{Difference,Intersection}{,Map,Multimap} and SetsUnion Refaster rules (#607) May 5, 2023

rickie changed the title ~~Introduce Sets{Difference,Intersection}{,Map,Multimap} and SetsUnion Refaster rules (#607)~~ Introduce Sets{Difference,Intersection}{,Map,Multimap} and SetsUnion Refaster rules May 5, 2023

philleonard pushed a commit that referenced this pull request May 26, 2023

Introduce Sets{Difference,Intersection}{,Map,Multimap} and `SetsUni…

671bc5d

…on` Refaster rules (#607)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `Sets{Difference,Intersection}{,Map,Multimap}` and `SetsUnion` Refaster rules #607

Introduce `Sets{Difference,Intersection}{,Map,Multimap}` and `SetsUnion` Refaster rules #607

mohamedsamehsalah commented May 1, 2023 •

edited

Loading

github-actions bot commented May 1, 2023

rickie left a comment •

edited by Stephan202

Loading

rickie May 2, 2023

Stephan202 May 2, 2023

github-actions bot commented May 2, 2023

Stephan202 left a comment

Stephan202 May 2, 2023

Stephan202 May 2, 2023

mohamedsamehsalah May 2, 2023

Stephan202 May 2, 2023

Stephan202 May 2, 2023

mohamedsamehsalah May 2, 2023 •

edited

Loading

Stephan202 May 2, 2023

github-actions bot commented May 2, 2023

mohamedsamehsalah commented May 2, 2023

Stephan202 left a comment

github-actions bot commented May 2, 2023

werli left a comment

github-actions bot commented May 3, 2023

sonarqubecloud bot commented May 3, 2023

Introduce Sets{Difference,Intersection}{,Map,Multimap} and SetsUnion Refaster rules #607

Introduce Sets{Difference,Intersection}{,Map,Multimap} and SetsUnion Refaster rules #607

Conversation

mohamedsamehsalah commented May 1, 2023 • edited Loading

github-actions bot commented May 1, 2023

rickie left a comment • edited by Stephan202 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedsamehsalah May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2023

mohamedsamehsalah commented May 2, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2023

werli left a comment

Choose a reason for hiding this comment

github-actions bot commented May 3, 2023

sonarqubecloud bot commented May 3, 2023

Introduce `Sets{Difference,Intersection}{,Map,Multimap}` and `SetsUnion` Refaster rules #607

Introduce `Sets{Difference,Intersection}{,Map,Multimap}` and `SetsUnion` Refaster rules #607

mohamedsamehsalah commented May 1, 2023 •

edited

Loading

rickie left a comment •

edited by Stephan202

Loading

mohamedsamehsalah May 2, 2023 •

edited

Loading