Improve and extend Refaster `Map` rules #337

Stephan202 · 2022-11-05T15:00:16Z

Suggested commit message:

Improve and extend Refaster `Map` rules (#337)

Summary of changes:
- Move relevant rules from `AssertJRules` to `AssertJMapRules` and introduce 
  associated tests.
- Extract relevant rules from `AssortedRules` to the new `MapRules` Refaster 
  rule collection.
- Add a few new rules to both `AssertJMapRules` and `MapRules`.

Stephan202 · 2022-11-05T15:01:02Z

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

 import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;

 @OnlineDocumentation
 final class AssertJMapRules {
  private AssertJMapRules() {}

+  // XXX: Reduce boilerplate using a `Matcher` that identifies "empty" instances.


I seem to recall that I already started working on this, but can't find the code right now. Will look more closely "later".

Stephan202 · 2022-11-05T15:05:21Z

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

+  static final class AssertThatMapHasSize<K, V> {
+    @BeforeTemplate
+    AbstractAssert<?, ?> before(Map<K, V> map, int length) {
+      return Refaster.anyOf(
+          assertThat(map.size()).isEqualTo(length),
+          assertThat(Refaster.anyOf(map.keySet(), map.values(), map.entrySet())).hasSize(length));
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    MapAssert<K, V> after(Map<K, V> map, int length) {
+      return assertThat(map).hasSize(length);
+    }
+  }
+
+  static final class AbstractMapAssertHasSameSizeAs<K, V> {
+    @BeforeTemplate
+    AbstractMapAssert<?, ?, K, V> before(AbstractMapAssert<?, ?, K, V> mapAssert, Map<K, V> map) {
+      return mapAssert.hasSize(map.size());
+    }
+
+    @AfterTemplate
+    AbstractMapAssert<?, ?, K, V> after(AbstractMapAssert<?, ?, K, V> mapAssert, Map<K, V> map) {
+      return mapAssert.hasSameSizeAs(map);
+    }
+  }


Part 1 of what I mentioned here.

Stephan202 · 2022-11-05T15:05:37Z

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

+  /** Prefer {@link Map#size()} over more contrived alternatives. */
+  static final class MapSize<K, V> {
+    @BeforeTemplate
+    int before(Map<K, V> map) {
+      return Refaster.anyOf(map.keySet(), map.values(), map.entrySet()).size();
+    }
+
+    @AfterTemplate
+    int after(Map<K, V> map) {
+      return map.size();
+    }
+  }


... and part 2.

werli

Quick Github-based review. The map-related rules LGTM, but the PR looks to also revert #327. Let's make sure we don't. 🙂

werli · 2022-11-06T10:23:36Z

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

-   * Avoid unnecessary {@link Optional} to {@link Stream} conversion when filtering a value of the
-   * former type.
-   */
-  static final class OptionalFilter<T> {


I suspect removing these two new Refaster rules in unintended? 🙂

Nice catch! Likely an error while moving the code to a new branch (I initially wrote this on top of #332).

Ptijohn · 2022-11-07T15:42:10Z

Note while reviewing, I find that doing some vimdiffs on the test files helps a lot to review the rules introduced :)

vimdiff error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestInput.java error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestOutput.java

vimdiff error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/MapRulesTestInput.java error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/MapRulesTestOutput.java

Ptijohn

One probably unrelated comment, the whole set of refactoring + adding of new rules suits me!
Thanks :)

Ptijohn · 2022-11-07T15:50:57Z

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

-  // same, so such a rule requires a `Matcher` that heuristically identifies `Stream` expressions
-  // with deterministic order.
+  // XXX: Consider whether to have a similar rule for `.findAny()`. For parallel streams it
+  // wouldn't be quite the same....


Suggested change

// wouldn't be quite the same....

// wouldn't be quite the same...

But did you even intend to drop the rest of the comment?

rickie

Added a commit. (Forgot to make a comment and already pushed the improvement 😛).

Changes LGTM.
Tweaked suggested commit message.

@werli anything else from you side?

rickie · 2022-11-08T08:36:27Z

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

 import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;

 @OnlineDocumentation
 final class AssertJMapRules {
  private AssertJMapRules() {}

+  // XXX: Reduce boilerplate using a `Matcher` that identifies "empty" instances.
+  static final class AbstractMapAssertIsEmpty<K, V> {


I was about to propose Javadocs for these templates. However, for most AssertJ related rules we don't have that 👀.

Summary of changes: - Move relevant rules from `AssertJRules` to `AssertJMapRules` and introduce associated tests. - Extract relevant rules from `AssortedRules` to the new `MapRules` Refaster collection. - Add a few new rules to both `AssertJMapRules` and `MapRules`.

rickie

Haha if you see the last commit @Stephan202 I know what you'll think, and you are right; we should add something for this to automate it 😛.

I was still thinking about why it wasn't flagged that it was Assorted instead of Map in the tests 😂, anyway it's fixed now :).

Stephan202 · 2022-11-08T14:51:03Z

I know what you'll think, and you are right; we should add something for this to automate it stuck_out_tongue.

Exactly 😉

Changes LGTM!

werli

Pushed my small suggestion. 👍 Nice improvements. 🙂

werli · 2022-11-08T17:32:36Z

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

-  // XXX: Implement a similar rule for `.findAny()`. For parallel streams this wouldn't be quite the
-  // same, so such a rule requires a `Matcher` that heuristically identifies `Stream` expressions
-  // with deterministic order.
+  // XXX: Consider whether to have a similar rule for `.findAny()`. For parallel streams it
+  // wouldn't be quite the same...


That's the last bit of unintended changes in this PR. 👀
See https://github.com/PicnicSupermarket/error-prone-support/pull/327/files#diff-35c5c43be02c59bf4ed5296465d36cc5adc02addcf0269f7264332cde0f0d779R171-R173

Stephan202 added this to the 0.6.0 milestone Nov 5, 2022

Stephan202 requested review from Ptijohn and rickie November 5, 2022 15:00

Stephan202 commented Nov 5, 2022

View reviewed changes

Stephan202 mentioned this pull request Nov 5, 2022

Expand AssertJ enumerable templates with Map support #332

Closed

werli reviewed Nov 6, 2022

View reviewed changes

mlrprananta mentioned this pull request Nov 7, 2022

Introduce MapIsEmpty Refaster rule #339

Merged

Ptijohn approved these changes Nov 7, 2022

View reviewed changes

rickie approved these changes Nov 8, 2022

View reviewed changes

rickie added the new feature label Nov 8, 2022

rickie self-requested a review November 8, 2022 09:11

Stephan202 and others added 4 commits November 8, 2022 10:14

Oh boy

edbe0b0

Suggestions

98dec76

Thats why it wasnt flagged... :)

02397eb

rickie force-pushed the sschroevers/refaster-map-improvements branch from 0b38be5 to 02397eb Compare November 8, 2022 09:14

rickie approved these changes Nov 8, 2022

View reviewed changes

Revert unintended changes

2e27c78

werli approved these changes Nov 8, 2022

View reviewed changes

rickie merged commit fcaa1f7 into master Nov 9, 2022

rickie deleted the sschroevers/refaster-map-improvements branch November 9, 2022 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve and extend Refaster `Map` rules #337

Improve and extend Refaster `Map` rules #337

Stephan202 commented Nov 5, 2022 •

edited by rickie

Loading

Stephan202 Nov 5, 2022

Stephan202 Nov 5, 2022

Stephan202 Nov 5, 2022

werli left a comment

werli Nov 6, 2022

Stephan202 Nov 6, 2022

Ptijohn commented Nov 7, 2022

Ptijohn left a comment

Ptijohn Nov 7, 2022

rickie left a comment •

edited

Loading

rickie Nov 8, 2022

rickie left a comment

Stephan202 commented Nov 8, 2022

werli left a comment

werli Nov 8, 2022

Stephan202 Nov 8, 2022

	// wouldn't be quite the same....
	// wouldn't be quite the same...

Improve and extend Refaster Map rules #337

Improve and extend Refaster Map rules #337

Conversation

Stephan202 commented Nov 5, 2022 • edited by rickie Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ptijohn commented Nov 7, 2022

Ptijohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Stephan202 commented Nov 8, 2022

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve and extend Refaster `Map` rules #337

Improve and extend Refaster `Map` rules #337

Stephan202 commented Nov 5, 2022 •

edited by rickie

Loading

rickie left a comment •

edited

Loading