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.

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

s/templates/rules/ and s/template/rule for all files and classes #288

Closed
wants to merge 8 commits into from

Conversation

rickie
Copy link
Member

@rickie rickie commented Oct 10, 2022

❗This PR is on top of #286

This should be the last part of all the changes. Went over all the occurrences of "template{,s}" thrice. Could still be the case that I missed one 👀:smile:.

@nathankooij
Copy link
Contributor

nathankooij commented Oct 11, 2022

@rickie it was a lot easier for me to review the set of changes as a whole. These are the ones I found:

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java
index 930baee2..ee3b4e11 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java
@@ -51,7 +51,7 @@ public final class RefasterTemplateModifiers extends BugChecker
   @Override
   public Description matchClass(ClassTree tree, VisitorState state) {
     if (!hasMatchingMember(tree, BEFORE_TEMPLATE_METHOD, state)) {
-      /* This class does not contain a Refaster template. */
+      /* This class does not contain a Refaster rule. */
       return Description.NO_MATCH;
     }
 
@@ -85,8 +85,8 @@ public final class RefasterTemplateModifiers extends BugChecker
 
     if (!hasMatchingMember(tree, PLACEHOLDER_METHOD, state)) {
       /*
-       * Templates without a `@Placeholder` method should be `final`. Note that Refaster enforces
-       * that `@Placeholder` methods are `abstract`, so templates _with_ such a method will
+       * Rules without a `@Placeholder` method should be `final`. Note that Refaster enforces
+       * that `@Placeholder` methods are `abstract`, so rules _with_ such a method will
        * naturally be `abstract` and non-`final`.
        */
       modifiersToAdd.add(Modifier.FINAL);
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java
index b8679cb7..f14b11ca 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java
@@ -491,7 +491,7 @@ final class AssertJRules {
   }
 
   // XXX: This overload is here because `assertThat` has an overload for `Comparable` types.
-  // Unfortunately this still doesn't convince Refaster to match this template in the context of
+  // Unfortunately this still doesn't convince Refaster to match this rule in the context of
   // Comparable types. Figure out why! Note that this also affects the `AssertThatOptional` rule.
   static final class AssertThatIterableHasOneComparableElementEqualTo<
       S extends Comparable<? super S>, T extends S> {
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java
index 61e0141a..4d1484b4 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJThrowingCallableRules.java
@@ -429,7 +429,7 @@ final class AssertJThrowingCallableRules {
     }
   }
 
-  // XXX: Drop this template in favour of a generic Error Prone check that flags
+  // XXX: Drop this rule in favour of a generic Error Prone check that flags
   // `String.format(...)` arguments to a wide range of format methods.
   static final class AbstractThrowableAssertHasMessage {
     @BeforeTemplate
@@ -449,7 +449,7 @@ final class AssertJThrowingCallableRules {
     }
   }
 
-  // XXX: Drop this template in favour of a generic Error Prone check that flags
+  // XXX: Drop this rule in favour of a generic Error Prone check that flags
   // `String.format(...)` arguments to a wide range of format methods.
   static final class AbstractThrowableAssertWithFailMessage {
     @BeforeTemplate
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java
index e4f7e93a..32731290 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java
@@ -117,7 +117,7 @@ final class AssortedRules {
   }
 
   /** Don't unnecessarily repeat boolean expressions. */
-  // XXX: This template captures only the simplest case. `@AlsoNegation` doesn't help. Consider
+  // XXX: This rule captures only the simplest case. `@AlsoNegation` doesn't help. Consider
   // contributing a Refaster patch, which handles the negation in the `@BeforeTemplate` more
   // intelligently.
   static final class LogicalImplication {
@@ -172,7 +172,7 @@ final class AssortedRules {
    * Collections#disjoint(Collection, Collection)}.
    */
   // XXX: Other copy operations could be elided too, but these are most common after application of
-  // the `DisjointSets` template defined above. If we ever introduce a generic "makes a copy"
+  // the `DisjointSets` rule defined above. If we ever introduce a generic "makes a copy"
   // stand-in, use it here.
   static final class DisjointCollections<T> {
     @BeforeTemplate
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java
index 56171d94..2d190698 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java
@@ -313,7 +313,7 @@ final class ImmutableMapRules {
     }
   }
 
-  // XXX: Add a template for this:
+  // XXX: Add a rule for this:
   // Maps.transformValues(streamOfEntries.collect(groupBy(fun)), ImmutableMap::copyOf)
   // ->
   // streamOfEntries.collect(groupBy(fun, toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)))
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java
index a1f46b72..7c990060 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java
@@ -78,8 +78,8 @@ final class OptionalRules {
   }
 
   /** Prefer {@link Optional#orElseThrow()} over the less explicit {@link Optional#get()}. */
-  // XXX: This template is analogous to `OptionalOrElseThrow` above. Arguably this is its
-  // generalization. If/when Refaster is extended to understand this, delete the template above.
+  // XXX: This rule is analogous to `OptionalOrElseThrow` above. Arguably this is its
+  // generalization. If/when Refaster is extended to understand this, delete the rule above.
   static final class OptionalOrElseThrowMethodReference<T> {
     @BeforeTemplate
     Function<Optional<T>, T> before() {
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java
index 193f2c4c..3e01463f 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java
@@ -128,8 +128,8 @@ final class StringRules {
    * Prefer direct delegation to {@link String#valueOf(Object)} over the indirection introduced by
    * {@link Objects#toString(Object)}.
    */
-  // XXX: This template is analogous to `StringValueOf` above. Arguably this is its generalization.
-  // If/when Refaster is extended to understand this, delete the template above.
+  // XXX: This rule is analogous to `StringValueOf` above. Arguably this is its generalization.
+  // If/when Refaster is extended to understand this, delete the rule above.
   static final class StringValueOfMethodReference {
     @BeforeTemplate
     Function<Object, String> before() {
diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java
index 55a99701..27c7791c 100644
--- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java
+++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java
@@ -65,9 +65,9 @@ final class RefasterRulesTest {
           TimeRules.class,
           WebClientRules.class);
 
-  // XXX: Create a JUnit extension to automatically discover the template collections in a given
+  // XXX: Create a JUnit extension to automatically discover the rule collections in a given
   // context to make sure the list is exhaustive.
-  private static Stream<Arguments> validateTemplateCollectionTestCases() {
+  private static Stream<Arguments> validateRuleCollectionTestCases() {
     // XXX: Drop the filter once we have added tests for AssertJ! We can then also replace this
     // method with `@ValueSource(classes = {...})`.
     return RULE_COLLECTIONS.stream()
@@ -75,9 +75,9 @@ final class RefasterRulesTest {
         .map(Arguments::arguments);
   }
 
-  @MethodSource("validateTemplateCollectionTestCases")
+  @MethodSource("validateRuleCollectionTestCases")
   @ParameterizedTest
-  void validateTemplateCollection(Class<?> clazz) {
+  void validateRuleCollection(Class<?> clazz) {
     RefasterRuleCollection.validate(clazz);
   }
 }
diff --git a/pom.xml b/pom.xml
index 47290da2..22def8ea 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1368,7 +1368,7 @@
                 <!-- XXX: Find a way to assert that test code (both inline
                 `BugChecker` test code and the Refaster test files) does not
                 exhibit anti-patterns other than those associated with the
-                check/template under test. Ideally all test cases are realistic. -->
+                check/rule under test. Ideally all test cases are realistic. -->
                 <error-prone.self-check-args>-XepAllSuggestionsAsWarnings -Xep:MethodReferenceUsage:OFF</error-prone.self-check-args>
             </properties>
             <build>
diff --git a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java
index 16fd5790..ce01ec99 100644
--- a/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java
+++ b/refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java
@@ -64,8 +64,7 @@ import tech.picnic.errorprone.refaster.runner.Refaster;
 @BugPattern(summary = "Exercises a Refaster rule collection", severity = ERROR)
 public final class RefasterRuleCollection extends BugChecker implements CompilationUnitTreeMatcher {
   private static final long serialVersionUID = 1L;
-  private static final String RULE_COLLECTION_FLAG =
-      "RefasterTemplateCollection:TemplateCollection";
+  private static final String RULE_COLLECTION_FLAG = "RefasterRuleCollection:RuleCollection";
   private static final String TEST_METHOD_NAME_PREFIX = "test";
 
   private final String ruleCollectionUnderTest;
@@ -139,7 +138,7 @@ public final class RefasterRuleCollection extends BugChecker implements Compilat
             .withPath(state.getPath()));
 
     ImmutableRangeMap<Integer, String> indexedMatches =
-        indexTemplateMatches(matches, ((JCCompilationUnit) tree).endPositions);
+        indexRuleMatches(matches, ((JCCompilationUnit) tree).endPositions);
 
     matches.forEach(state::reportMatch);
     reportMissingMatches(tree, indexedMatches, state);
@@ -171,7 +170,7 @@ public final class RefasterRuleCollection extends BugChecker implements Compilat
     }
   }
 
-  private static ImmutableRangeMap<Integer, String> indexTemplateMatches(
+  private static ImmutableRangeMap<Integer, String> indexRuleMatches(
       List<Description> matches, EndPosTable endPositions) {
     ImmutableRangeMap.Builder<Integer, String> ruleMatches = ImmutableRangeMap.builder();
 
@@ -282,7 +281,7 @@ public final class RefasterRuleCollection extends BugChecker implements Compilat
       }
 
       /*
-       * Unless this method is `RefasterTemplateTestCase#elidedTypesAndStaticImports`, it's
+       * Unless this method is `RefasterRuleCollectionTestCase#elidedTypesAndStaticImports`, it's
        * misnamed.
        */
       if (!"elidedTypesAndStaticImports".equals(methodName)) {
diff --git a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollectionTest.java b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollectionTest.java
index b707bb07..1f38eaea 100644
--- a/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollectionTest.java
+++ b/refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollectionTest.java
@@ -25,7 +25,7 @@ final class RefasterRuleCollectionTest {
         RuleWithoutTestRules.class,
         ValidRules.class
       })
-  void verifyRefasterTemplateCollections(Class<?> clazz) {
+  void verifyRefasterRuleCollections(Class<?> clazz) {
     RefasterRuleCollection.validate(clazz);
   }
 }

I'm not entirely sure how you want to go about applying this. Moreover, I wasn't sure if the RefasterTemplateModifiers class should be renamed as a whole.

rickie added a commit that referenced this pull request Oct 11, 2022
@rickie
Copy link
Member Author

rickie commented Oct 11, 2022

@nathankooij the changes from RefasterTemplateModifiers.java are there on purpose. There we specifically talk about templates, namely the before- and after-templates.

Besides that; in AssertJRules I feel we are talking about the actual before-template instead of the rule.

Thanks for finding the others, interesting to see I missed these comments 🤔.

@nathankooij
Copy link
Contributor

@nathankooij the changes from RefasterTemplateModifiers.java are there on purpose. There we specifically talk about templates, namely the before- and after-templates.

I realize it's a fine line, but some of the changes are on the rule level (e.g. adding final or removing abstract). I'll let you be the judge in that case. ;)

Besides that; in AssertJRules I feel we are talking about the actual before-template instead of the rule.

Yes, was in dubio about this one. It's talking about the matching logic in the before template, but that also kind of makes a statement about the matching of the rules as a whole. Can go either way on that one.

@nathankooij nathankooij changed the base branch from rossendrijver/refaster-templates-rules to master October 11, 2022 10:48
@nathankooij
Copy link
Contributor

Pushed my changes, dropping one change in the modifiers test.

@rickie
Copy link
Member Author

rickie commented Oct 11, 2022

Ah I updated the other branch to do the work there. So ported your commit to #286 , see a2182cf (#286).

@rickie
Copy link
Member Author

rickie commented Oct 11, 2022

Will close this one in favor of #286 .

@rickie rickie closed this Oct 11, 2022
rickie added a commit that referenced this pull request Oct 11, 2022
@Stephan202 Stephan202 deleted the rossendrijver/refaster-templates-rules-2 branch October 11, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants