Introduce `StringIs{,Not}EmptyPredicate` Refaster rules #577

mohamedsamehsalah · 2023-04-10T15:34:03Z

Original suggestion here.

Suggested commit message:

Prefer `not(String::isEmpty) over `s -> !s.isEmpty()` for Predicates

github-actions · 2023-04-10T15:35:49Z

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.

...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java

mohamedsamehsalah · 2023-04-10T15:50:46Z

...ne-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java

@@ -39,6 +39,8 @@ ImmutableSet<Boolean> testStringIsNullOrEmpty() {
        getClass().getName() != null && !getClass().getName().isEmpty());
  }

+  // XXX: Consider updating qualifier `Predicate#not` if/when conditional `ImportPolicy` is


Allow more control over which methods are statically imported by @UseImportPolicy. Sometimes the @AfterTemplate contains more than one static method invocation, and only a subset should be statically imported.

See here

Check. Effectively we won't do this, I think. Instead we'll just rely on the existing StaticImport check.

github-actions · 2023-04-10T15:51:40Z

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-04-10T15:51:55Z

...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java

-        Optional.ofNullable(toString()).filter(s -> !s.isEmpty()),
-        Optional.ofNullable(toString()).filter(s -> !s.isEmpty()));
+        Optional.ofNullable(toString()).filter(Predicate.not(String::isEmpty)),
+        Optional.ofNullable(toString()).filter(Predicate.not(String::isEmpty)),


Should be a candidate for static import later.
See here

Stephan202

Tnx for picking this up @mohamedsamehsalah! I added a commit with a proposal.

Suggested commit message:

Introduce `StringIs{,Not}EmptyPredicate` Refaster rules (#577)

Stephan202 · 2023-04-10T17:01:40Z

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

    }
  }

  static final class FilterEmptyString {
    @BeforeTemplate
    Optional<String> before(Optional<String> optional) {
-      return optional.map(Strings::emptyToNull);
+      return Refaster.anyOf(optional.map(Strings::emptyToNull), optional.filter(s -> !s.isEmpty()));


Now the rule is very specific to this case. Instead we can introduce a separate rule like we have for EqualsPredicate. (In general we always try to go the smallest/most generic rule.)

Stephan202 · 2023-04-10T17:03:16Z

...ne-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java

@@ -39,6 +39,8 @@ ImmutableSet<Boolean> testStringIsNullOrEmpty() {
        getClass().getName() != null && !getClass().getName().isEmpty());
  }

+  // XXX: Consider updating qualifier `Predicate#not` if/when conditional `ImportPolicy` is


Check. Effectively we won't do this, I think. Instead we'll just rely on the existing StaticImport check.

github-actions · 2023-04-10T17:15: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.

Stephan202 · 2023-04-10T17:52:17Z

error-prone-contrib/README.md

- Allow more control over _which_ methods are statically imported by
-  `@UseImportPolicy`. Sometimes the `@AfterTemplate` contains more than one
-  static method invocation, and only a subset should be statically imported.


Changed my mind about removing this, as I just noticed that this is under the heading "Refaster extension ideas". Will revert. (Actually moving this section elsewhere is for another day.)

(Actually moving this section elsewhere is for another day.)

I can pick that up if you tell me where it is suposed to be 😅

Haha, not sure. Eventually we should likely have additional sections on the website, but that requires first knowing what we want. (And even then the text should likely be reviewed first, etc.)

But I'll keep an eye out for other possible PRs! 😄

Alright, maybe you'd be interested in this one: #578 😄

Thank you 🚀

github-actions · 2023-04-10T17:54:56Z

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, curious to see the impact on our codebase 🚀 !

@werli can you take a look? 😄

rickie

I meant to press approve 😅.

github-actions · 2023-04-13T07:52:37Z

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 · 2023-04-15T15:01:28Z

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 · 2023-04-16T20:13:36Z

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

Let's indeed generalize this in MethodReferenceUsage. 👍

Small step for now, but should be solved on a larger scale.

github-actions · 2023-04-18T06:26:09Z

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 force-pushed the mohamedsamehsalah/prefer-string-filter-Predicate-not-isEmpty branch from 5b9e3e1 to b6d3c6a Compare April 10, 2023 15:50

mohamedsamehsalah commented Apr 10, 2023

View reviewed changes

...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java Outdated Show resolved Hide resolved

mohamedsamehsalah commented Apr 10, 2023

View reviewed changes

mohamedsamehsalah changed the title ~~Prefer not(String::isEmpty) over s -> !s.isEmpty()` when filtering~~ Prefer not(String::isEmpty) over s -> !s.isEmpty()` for Predicates Apr 10, 2023

mohamedsamehsalah requested review from Stephan202, werli and rickie April 10, 2023 16:09

Stephan202 approved these changes Apr 10, 2023

View reviewed changes

Stephan202 added this to the 0.10.0 milestone Apr 10, 2023

Stephan202 reviewed Apr 10, 2023

View reviewed changes

rickie reviewed Apr 13, 2023

View reviewed changes

rickie approved these changes Apr 13, 2023

View reviewed changes

rickie force-pushed the mohamedsamehsalah/prefer-string-filter-Predicate-not-isEmpty branch from 4d2cb98 to c316c88 Compare April 13, 2023 07:50

rickie added the new feature label Apr 13, 2023

rickie force-pushed the mohamedsamehsalah/prefer-string-filter-Predicate-not-isEmpty branch from c316c88 to 2e52821 Compare April 15, 2023 14:59

mohamedsamehsalah force-pushed the mohamedsamehsalah/prefer-string-filter-Predicate-not-isEmpty branch from 2e52821 to 2cceebb Compare April 16, 2023 20:12

werli approved these changes Apr 17, 2023

View reviewed changes

mohamedsamehsalah and others added 3 commits April 18, 2023 08:24

Prefer not(String::isEmpty) over s -> !s.isEmpty()` when filtering

2298efc

Suggstions

8c2f7c9

Revert README change

96b5773

mohamedsamehsalah force-pushed the mohamedsamehsalah/prefer-string-filter-Predicate-not-isEmpty branch from 2cceebb to 96b5773 Compare April 18, 2023 06:24

rickie changed the title ~~Prefer not(String::isEmpty) over s -> !s.isEmpty()` for Predicates~~ Introduce StringIs{,Not}EmptyPredicate Refaster rules Apr 18, 2023

rickie merged commit 3af81d8 into master Apr 18, 2023

rickie deleted the mohamedsamehsalah/prefer-string-filter-Predicate-not-isEmpty branch April 18, 2023 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `StringIs{,Not}EmptyPredicate` Refaster rules #577

Introduce `StringIs{,Not}EmptyPredicate` Refaster rules #577

mohamedsamehsalah commented Apr 10, 2023

github-actions bot commented Apr 10, 2023

mohamedsamehsalah Apr 10, 2023

Stephan202 Apr 10, 2023

github-actions bot commented Apr 10, 2023

mohamedsamehsalah Apr 10, 2023

Stephan202 left a comment

Stephan202 Apr 10, 2023

Stephan202 Apr 10, 2023

github-actions bot commented Apr 10, 2023

Stephan202 Apr 10, 2023

mohamedsamehsalah Apr 10, 2023

Stephan202 Apr 10, 2023

Stephan202 Apr 11, 2023

mohamedsamehsalah Apr 11, 2023

github-actions bot commented Apr 10, 2023

rickie left a comment

rickie left a comment

github-actions bot commented Apr 13, 2023

github-actions bot commented Apr 15, 2023

github-actions bot commented Apr 16, 2023

werli left a comment

github-actions bot commented Apr 18, 2023

Introduce StringIs{,Not}EmptyPredicate Refaster rules #577

Introduce StringIs{,Not}EmptyPredicate Refaster rules #577

Conversation

mohamedsamehsalah commented Apr 10, 2023

github-actions bot commented Apr 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 10, 2023

Choose a reason for hiding this comment

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

github-actions bot commented Apr 10, 2023

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

github-actions bot commented Apr 10, 2023

rickie left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 13, 2023

github-actions bot commented Apr 15, 2023

github-actions bot commented Apr 16, 2023

werli left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2023

Introduce `StringIs{,Not}EmptyPredicate` Refaster rules #577

Introduce `StringIs{,Not}EmptyPredicate` Refaster rules #577