Move files and resources to `.refastertemplates` packages #113

rickie · 2022-05-25T14:27:28Z

This commit is to prepare the code for #43.
Here we already move the templates and associated resources.

Note that for the RefasterCheckTest, RefasterTemplateTestCase, and RefasterCheckTest there are some temporary changes. In #43 most things will be moved to refaster-test-support or refaster-runner. They are currently moved as to make the existing state work.

rickie

Some context.

rickie · 2022-05-25T14:28:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java

@@ -196,10 +194,7 @@ private static ImmutableListMultimap<String, CodeTransformer> loadAllCodeTransfo

  private static ImmutableSet<ResourceInfo> getClassPathResources() {
    try {
-      return ClassPath.from(


This change is copied over from here.

rickie · 2022-05-25T14:28:55Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java

@@ -68,9 +66,9 @@ public final class RefasterCheck extends BugChecker implements CompilationUnitTr
  private static final String REFASTER_TEMPLATE_SUFFIX = ".refaster";
  private static final String INCLUDED_TEMPLATES_PATTERN_FLAG = "Refaster:NamePattern";

-  @VisibleForTesting


This will also change in the "end-state" implementation.

rickie · 2022-05-26T12:35:12Z

(Not yet fixed) 😄

rickie · 2022-05-26T18:11:12Z

Fixed now :).

Stephan202

Added two commits; TBD whether both make sense in the context of #43. (Hope to review that PR ~tomorrow.)

Stephan202 · 2022-05-28T06:51:56Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/refastertemplates/RefasterCheckTest.java

+import tech.picnic.errorprone.bugpatterns.RefasterCheck;

 public final class RefasterCheckTest {


So the test is now in a different package than the main class. As discussed, this is to work around a limitation of the current test setup, to be resolved in an upcoming PR. Let's add a comment to that effect.

Actually, we can avoid the move using another few-line change. Not elegant, but IMHO preferable to moving the class.

(TBD whether this insight allows simplifications in #43; didn't scrutinize that PR yet.)

I'm not sure whether I agree that this is better, but as is it a temporary state I'm OK with it 😄.

The RefasterCheck will go to the refaster-runner module in #43.

Stephan202 · 2022-05-28T06:52:09Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterCheck.java

-  @VisibleForTesting
-  static final Supplier<ImmutableListMultimap<String, CodeTransformer>> ALL_CODE_TRANSFORMERS =
-      Suppliers.memoize(RefasterCheck::loadAllCodeTransformers);
+  /** All code transformers loaded by {@link RefasterCheck}. */


Suggested change

/** All code transformers loaded by {@link RefasterCheck}. */

/** All code transformers found on the classpath, loaded lazily. */

Stephan202 · 2022-05-29T12:15:56Z

Build fails because of:

[INFO] --- maven-compiler-plugin:3.10.1:testCompile (default-testCompile) @ error-prone-contrib ---
[INFO] Compiling 28 source files to /home/sschroevers/workspace/picnic/error-prone-support/error-prone-contrib/target/test-classes
[INFO] -------------------------------------------------------------
[WARN] COMPILATION WARNING : 
[INFO] -------------------------------------------------------------
[WARN] /home/sschroevers/workspace/picnic/error-prone-support/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterCheckTest.java:[185,11] [BooleanParameter] Use parameter comments to document ambiguous literals
    (see https://errorprone.info/bugpattern/BooleanParameter)
  Did you mean '/* initialize= */false,'?
[INFO] 1 warning

I didn't see this error locally. Turns out that this error triggers when building with JDK 11, but not when building with JDK 17. In the latter case the BooleanParameter check backs off following this check, because Class#forName's parameters are reported as arg0, arg1 and arg2. I tried to "fix" this by adding --add-exports=java.base/java.lang=ALL-UNNAMED and --add-opens=java.base/java.lang=ALL-UNNAMED to .mvn/jvm.config, but that didn't help. Won't investigate further; added a commit to resolve the warning.

Stephan202

Suggested commit message:

Move Refaster template test resources to proper package (#113)

rickie · 2022-05-31T12:30:08Z

Made a tiny tweak to the suggested commit message to be more accurate.

Stephan202 · 2022-05-31T12:48:04Z

Right. Tweaked further ;)

Badbond

LGTM. Thought we could create a final member variable for BugCheckerRefactoringTestHelper but this didn't speed up tests much and I suspect will become a problem with Junit method parallelism.

Stephan202 · 2022-06-01T12:55:12Z

LGTM. Thought we could create a final member variable for BugCheckerRefactoringTestHelper but this didn't speed up tests much and I suspect will become a problem with Junit method parallelism.

Discussed offline. This is a good idea. However, as we'll anyway change this code in #43 we'll skip this change 👍

rickie requested review from Stephan202 and Badbond May 25, 2022 14:27

rickie commented May 25, 2022

View reviewed changes

rickie added this to the 0.1.0 milestone May 25, 2022

rickie force-pushed the rossendrijver/move_refastertemplates branch from 8269b7b to 7b8da2d Compare May 26, 2022 12:33

rickie force-pushed the rossendrijver/move_refastertemplates branch from 7b8da2d to 082d0de Compare May 26, 2022 18:10

Stephan202 reviewed May 28, 2022

View reviewed changes

Stephan202 mentioned this pull request May 29, 2022

Introduce validator for Refaster template collections and tests #43

Merged

rickie and others added 8 commits May 31, 2022 14:05

Move Refaster test resources to .refastertemplates package

0186877

Move the RefasterTemplateTestCase to .refastertemplates

4a8ac65

Use SystemClassLoader in RefasterCheck

44d26c0

Move RefasterCheckTest to .refastertemplates package

c8fde87

Post-rebase fix

51b4e67

Suggestions

2051b50

Alternative suggestion

2ad8f86

Resolve warning

b31a789

rickie force-pushed the rossendrijver/move_refastertemplates branch from 9588d46 to b31a789 Compare May 31, 2022 12:11

Stephan202 approved these changes May 31, 2022

View reviewed changes

Badbond approved these changes Jun 1, 2022

View reviewed changes

Stephan202 merged commit 8277b43 into master Jun 1, 2022

Stephan202 deleted the rossendrijver/move_refastertemplates branch June 1, 2022 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move files and resources to `.refastertemplates` packages #113

Move files and resources to `.refastertemplates` packages #113

rickie commented May 25, 2022

rickie left a comment

rickie May 25, 2022

rickie May 25, 2022

rickie commented May 26, 2022

rickie commented May 26, 2022

Stephan202 left a comment

Stephan202 May 28, 2022

Stephan202 May 28, 2022 •

edited

Loading

rickie May 31, 2022

rickie May 31, 2022

Stephan202 May 28, 2022

Stephan202 commented May 29, 2022

Stephan202 left a comment •

edited

Loading

rickie commented May 31, 2022

Stephan202 commented May 31, 2022

Badbond left a comment

Stephan202 commented Jun 1, 2022

		import tech.picnic.errorprone.bugpatterns.RefasterCheck;

		public final class RefasterCheckTest {

	/** All code transformers loaded by {@link RefasterCheck}. */
	/** All code transformers found on the classpath, loaded lazily. */

Move files and resources to .refastertemplates packages #113

Move files and resources to .refastertemplates packages #113

Conversation

rickie commented May 25, 2022

rickie left a comment

Choose a reason for hiding this comment

rickie May 25, 2022

Choose a reason for hiding this comment

rickie May 25, 2022

Choose a reason for hiding this comment

rickie commented May 26, 2022

rickie commented May 26, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 May 28, 2022

Choose a reason for hiding this comment

Stephan202 May 28, 2022 • edited Loading

Choose a reason for hiding this comment

rickie May 31, 2022

Choose a reason for hiding this comment

rickie May 31, 2022

Choose a reason for hiding this comment

Stephan202 May 28, 2022

Choose a reason for hiding this comment

Stephan202 commented May 29, 2022

Stephan202 left a comment • edited Loading

Choose a reason for hiding this comment

rickie commented May 31, 2022

Stephan202 commented May 31, 2022

Badbond left a comment

Choose a reason for hiding this comment

Stephan202 commented Jun 1, 2022

Move files and resources to `.refastertemplates` packages #113

Move files and resources to `.refastertemplates` packages #113

Stephan202 May 28, 2022 •

edited

Loading

Stephan202 left a comment •

edited

Loading