Emit website link along with Refaster refactor suggestions #255

Stephan202 · 2022-09-21T20:20:27Z

❗ ~~This PR is on top of #286~~ ❗

Work in progress; see this thread for context.

japborst · 2022-09-22T11:19:08Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java

+      // specify an optional URL pattern.)
+      // XXX: Replace only the first `$`.
+      // XXX: Review URL format. Currently produced format:
+      // https://error-prone.picnic.tech/refastertemplates/OptionalTemplates#OptionalOrElseThrow


This URL looks great 😍 FYI @oxkitsune for the appropriate anchors to use.

Stephan202

Added a commit with some early thoughts about supporting custom annotations on Refaster template. Code is very rough, but I can see in a debugger that this works at least in principle.

@rickie We could also use this annotations for more fine-grained filtering, e.g. based on whether a property is behavior preserving.

@oxkitsune these annotations could be used for documentation generation :)

Stephan202 · 2022-09-22T16:08:23Z

...ter-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java

@@ -0,0 +1,12 @@
+package tech.picnic.errorprone.refaster.annotation;


We might want to give the package tech.picnic.errorprone.refaster.util a better name. Maybe .matchers.

I'm not sure I agree with that. We can introduce a .matchers but not all things would go in there. The SourceCode and JavaKeywords are really just utils IMO.

Ah, I see I was looking in the wrong place. I was confused because of the name TemplateCollection. Yes you are absolutely right, we should rename this. Can be part of this PR right? I'll do it anyway, if you think it should be separate we can move the commit out anyways.

Filed: #267

...ter-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TemplateCollection.java

Stephan202

A few open points remain, but I don't expect the over-all structure of the PR to change much. So reviews are welcome.

Stephan202 · 2022-09-24T21:07:25Z

...-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java


 /** Refaster templates related to expressions dealing with {@link Optional}s. */
+@OnlineDocumentation


The idea is that we add this annotation to each Refaster template collection in this repository.

But let's do that in a follow-up PR right?

Either-or. (As in: we drop it here, or add it to the others. I'm fine with deferring in order to keep the PR smaller. Ideally we automate the task, perhaps reusing some of the code I'm writing in this context. TBD.)

Stephan202 · 2022-09-24T21:08:11Z

...-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java

@@ -63,7 +66,7 @@ boolean after(Optional<T> optional) {
    }
  }

-  /** Prefer {@link Optional#orElseThrow()} over the less explicit {@link Optional#get()}. */
+  @Description("Prefer `Optional#orElseThrow()` over the less explicit `Optional#get()`")


@oxkitsune (once applied more widely) these annotations can be used for the website generation.

Stephan202 · 2022-09-24T21:10:25Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooTemplates.java

+  /** A simple template for testing purposes, having several custom annotations. */
+  @Description("A custom description about matching single-char strings")
+  @OnlineDocumentation
+  @Severity(WARNING)


The default severity is SUGGESTION. @oxkitsune this @Severity annotation can also be used for website generation. (Not yet sure whether we have (m)any Refaster templates that deserve a severity override, but still.)

Stephan202 · 2022-09-28T15:04:01Z

Rebased; PR now targets #267.

rickie

Added a commit with a proposal to solve the XXXs related to -XepAllSuggestionsAsWarnings, PTAL. I think we can mark this as ready for review because most XXXs are now picked up.

The XXX about testing AnnotatedCompositeCodeTransformer I want to propose to defer. Especially because we need this logic for the other PRs. We can later add the test for sure.

rickie · 2022-09-29T08:37:09Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java

+/**
+ * Describes the severity of a Refaster template or group of Refaster templates.
+ *
+ * <p>The default severity is {@link SeverityLevel#SUGGESTION}. Annotations on nested classes


It's a bit unnatural that we say the default is SeverityLevel#SUGGESTION but below we do not specify that.

Although, looking at the other code, I can see why it is. That wasn't my point :).

Yeah, there are some delicate things going on here. I'll push a proposal.

rickie · 2022-09-29T08:38:01Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java

+  private static String extractRefasterTemplateName(Description description) {
+    String message = description.getRawMessage();
+    int index = message.indexOf(':');
+    checkState(index >= 0, "String '%s' does not contain character '%s'", message, ':');


Suggested change

checkState(index >= 0, "String '%s' does not contain character '%s'", message, ':');

checkState(index >= 0, "String '%s' does not contain character ':', message);

Can't we simplify this? Will push the proposal.

rickie · 2022-09-29T08:44:14Z

.../src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java

+// XXX: Can we find a better name for thi class?
+// XXX: Use `@AutoValue`?
+// XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.)
+final class AnnotatedCompositeCodeTransformer implements CodeTransformer, Serializable {


Hmm, will throw in one other idea for now: AugmentedCompositeCodeTransformer. We use augment in this context a few times. It's hard to come up with a better name I'd say. So will probably remove that XXX as I like the current name.

rickie · 2022-09-29T18:30:13Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

+      Description description, Optional<SeverityLevel> severityOverride) {
+    return Description.builder(
+            description.position,
+            "Refaster Rule",


Suggested change

"Refaster Rule",

"RefasterRule",

👀 All checks are without space, so would propose to do the same here TBH.

That space is there very much on purpose:

The replacement check name ("Refaster Template") is chosen such that it is guaranteed not to match any canonical bug checker name (as that could cause {@link VisitorState#reportMatch(Description)}} to override the reported severity).

I'll make it more explicit.

Currently the value (Refaster Rule) does not match the documentation (Refaster Template). Changed the documentation to the former.

We use both Refaster Rule and Refaster Template a lot. Not really sure if we need to settle on using one of the two.
Now it depends a bit on the situation, I feel.

We use both Refaster Rule and Refaster Template a lot. Not really sure if we need to settle on using one of the two.
Now it depends a bit on the situation, I feel.

To be honest, I try to only use the word "template" to avoid confusing and make people use the same words. I'd be okay with switching everything to template. WDYT?

Can't find a clear distinction between a rule and a template in the original documentation either. I'm happy with both (slightly prefer template, given the annotations), as long as we are consistent 👍

Yeah, this has bugged me for a while as well. I suspect it's more correct to call "the whole thing" a Refaster rule, since (a) it's represented by the RefasterRule type and (b) the individual methods are called before/after templates.

So that would make "Refaster Rule" correct for this code.

That still leaves many incorrect usages elsewhere in the code base... maybe something for a separate PR?

Yea let's defer to another PR.

I'm also in camp let's be consistent as @Badbond is haha. I bet you are as well.
It makes slightly more sense to say "lets add a rule for that" instead of "lets add a template for that".

rickie

Not done reviewing, flushing what I have so far.

rickie · 2022-09-29T19:03:26Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java

+          .expectErrorMessage(
+              "StringOfSizeZeroTemplate",
+              containsPattern(
+                  "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeZeroTemplate: Refactoring opportunity\\s+.+\\s+null"))


Did you perhaps see where the null is coming from? We can look into that later :).

Yeah, it's due to this code calling this method. Could be fixed by unconditionally calling linkTextForDiagnostic, and then discarding the result if null. Wanna open a PR upstream? :)

According to the Description.Builder, link is Nullable. We could instead of an empty string pass in null at AnnotatedCompositeCodeTransformer#augmentDescription if we don't have a url, causing this to not appear at all (verified this). This might be nice to do in this transition period until all checks and templates have a link. Got a commit ready if we wish to do this.

Edit: pushed after having it discussed offline with @Stephan202. PTAL and feel free to tweak.

Wanna open a PR upstream? :)

Yes that sounds good. Would be a nice improvement.
Will write it down and do after the EPS OSS-ing journey 😛.

rickie · 2022-09-29T19:06:30Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java

+/**
+ * Describes the severity of a Refaster template or group of Refaster templates.
+ *
+ * <p>The default severity is {@link SeverityLevel#SUGGESTION}. Annotations on nested classes


Although, looking at the other code, I can see why it is. That wasn't my point :).

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java

.../src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java

Stephan202

Rebased and added a commit.

pom.xml

Stephan202 · 2022-09-29T18:19:57Z

.../src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java

+import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
+import tech.picnic.errorprone.refaster.annotation.Severity;
+
+// XXX: Test this class directly. (Right now it's only indirectly tested through `RefasterTest`.)


I see the comment was dropped, but the naming question is still open for me (I don't quite like the current name).

Stephan202 · 2022-09-29T20:49:16Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java

+    int index = message.indexOf(':');
+    checkState(index >= 0, "String '%s' does not contain character ':'", message);


Maybe we don't even need to mention :.

Stephan202 · 2022-09-29T20:51:49Z

.../src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java

+      Optional<Method> isSuggestionsAsWarningsMethod =
+          Arrays.stream(errorProneOptions.getClass().getDeclaredMethods())
+              .filter(m -> Modifier.isPublic(m.getModifiers()))
+              .filter(m -> m.getName().equals("isSuggestionsAsWarnings"))
+              .findFirst();
+      return isSuggestionsAsWarningsMethod.isPresent()


This is classpath-invariant, so we should execute this code only once.

Stephan202 · 2022-09-29T20:52:34Z

refaster-runner/pom.xml

+            <!-- XXX: This `runtime` scope declaration ensures that
+            `AnnotatedCompositeCodeTransformer` instances can be deserialized.
+            It also makes our custom Refaster annotations available. The issue
+            with this setup is that `refaster-compiler` has *non*-`provided`
+            dependency declarations on Error Prone, as it should be able to
+            function independently. This could cause classpath issues for users
+            of *this* module. Consider moving
+            `AnnotatedCompositeCodeTransformer` back to `refaster-support`. -->
+            <scope>runtime</scope>


Note to self: fix this before merging the PR.

Stephan202 · 2022-09-29T21:12:00Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java

+    /* { arguments, expectedSeverities } */
+
+    Stream<Arguments> forkTestCases =
+        isBuiltWithErrorProneFork()


"Built" is past tense, while this is about the current classpath :)

Stephan202 · 2022-09-29T21:12:20Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java

+  private static boolean isBuiltWithErrorProneFork() {
+    Class<?> clazz;
+    try {
+      clazz =
+          Class.forName(
+              "com.google.errorprone.ErrorProneOptions",
+              /* initialize= */ false,
+              Thread.currentThread().getContextClassLoader());
+    } catch (ClassNotFoundException e) {
+      throw new IllegalStateException("Can't load `ErrorProneOptions` class", e);
+    }
+    return Arrays.stream(clazz.getDeclaredMethods())
+        .filter(m -> Modifier.isPublic(m.getModifiers()))
+        .anyMatch(m -> m.getName().equals("isSuggestionsAsWarnings"));
+  }


Let's reuse the other code; will push a proposal.

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/package-info.java

Stephan202 · 2022-09-29T21:31:17Z

...-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java


 /** Refaster templates related to expressions dealing with {@link Optional}s. */
+@OnlineDocumentation


Here and below: don't forget to revert the changes in this file, assuming we introduce the annotations separately.

Stephan202

Added a commit.

Stephan202 · 2022-09-30T05:00:33Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Severity.java

+/**
+ * Describes the severity of a Refaster template or group of Refaster templates.
+ *
+ * <p>The default severity is {@link SeverityLevel#SUGGESTION}. Annotations on nested classes


Yeah, there are some delicate things going on here. I'll push a proposal.

Stephan202 · 2022-09-30T05:53:50Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

+      Description description, Optional<SeverityLevel> severityOverride) {
+    return Description.builder(
+            description.position,
+            "Refaster Rule",


That space is there very much on purpose:

The replacement check name ("Refaster Template") is chosen such that it is guaranteed not to match any canonical bug checker name (as that could cause {@link VisitorState#reportMatch(Description)}} to override the reported severity).

I'll make it more explicit.

Stephan202 · 2022-09-30T05:58:29Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java

+          .expectErrorMessage(
+              "StringOfSizeZeroTemplate",
+              containsPattern(
+                  "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeZeroTemplate: Refactoring opportunity\\s+.+\\s+null"))


Yeah, it's due to this code calling this method. Could be fixed by unconditionally calling linkTextForDiagnostic, and then discarding the result if null. Wanna open a PR upstream? :)

Stephan202 · 2022-09-30T05:59:42Z

.../src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java

+import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
+import tech.picnic.errorprone.refaster.annotation.Severity;
+
+// XXX: Can we find a better name for this class? `CompositeAnnotatedCodeTransformer`, ...?


I missed this comment. I think I did at some point also ponder AugmentedCompositeCodeTransformer. Might still be the way to go; will add it to the list.

Badbond

Flushing first part of review and added a commit.

Badbond · 2022-09-30T08:24:50Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

+      Description description, Optional<SeverityLevel> severityOverride) {
+    return Description.builder(
+            description.position,
+            "Refaster Rule",


Currently the value (Refaster Rule) does not match the documentation (Refaster Template). Changed the documentation to the former.

Badbond · 2022-09-30T09:31:38Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

+   * VisitorState#reportMatch(Description)} will override the reported severity only if this bug
+   * checker's severity was explicitly configured.


For my understanding (and for future readers), we currently do not support overriding the severity level of a set of Refaster templates, right? This would require the parsing of a new flag.

Indeed, one can only override the default for all templates. I do see now that this sentence is no longer correct (it reflects an earlier understanding I had of the situation), so will push a tweak.

Badbond · 2022-09-30T12:33:15Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java

+          .expectErrorMessage(
+              "StringOfSizeZeroTemplate",
+              containsPattern(
+                  "\\[Refaster Rule\\] FooTemplates\\.StringOfSizeZeroTemplate: Refactoring opportunity\\s+.+\\s+null"))


According to the Description.Builder, link is Nullable. We could instead of an empty string pass in null at AnnotatedCompositeCodeTransformer#augmentDescription if we don't have a url, causing this to not appear at all (verified this). This might be nice to do in this transition period until all checks and templates have a link. Got a commit ready if we wish to do this.

Edit: pushed after having it discussed offline with @Stephan202. PTAL and feel free to tweak.

Badbond · 2022-09-30T13:18:27Z

refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/ErrorProneFork.java

+   *
+   * @param options The currently active Error Prone options.
+   * @return {@code true} iff {@link #isErrorProneForkAvailable() the Error Prone fork is available}
+   *     and the aforementioned flag is configured.


Suggested change

* and the aforementioned flag is configured.

* and the aforementioned flag is set.

In line with the subject, otherwise it is a bit ambigious as this could also indicate one can configure the flag.

Stephan202

Tnx for the review so far @Badbond! Added a small commit :)

Stephan202 · 2022-09-30T19:55:32Z

.../src/main/java/tech/picnic/errorprone/refaster/plugin/AnnotatedCompositeCodeTransformer.java

+        .map(
+            urlPattern ->
+                urlPattern.replace(TOP_LEVEL_CLASS_URL_PLACEHOLDER, nameComponents.next()))
+        .map(
+            urlPattern ->
+                urlPattern.replace(
+                    NESTED_CLASS_URL_PLACEHOLDER, Iterators.getNext(nameComponents, "")));


Since the variables have a small scope anyway: let's rename to url and avoid the line wrapping.

Stephan202 · 2022-09-30T19:57:53Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

+   * VisitorState#reportMatch(Description)} will override the reported severity only if this bug
+   * checker's severity was explicitly configured.


Indeed, one can only override the default for all templates. I do see now that this sentence is no longer correct (it reflects an earlier understanding I had of the situation), so will push a tweak.

rickie

Sooo, the open points for now (that are a MUST before merging) are:

Settle on a name for the AnnotatedCompositeCodeTransformer.
Revert the changes in OptionalTemplates.
Fixing the runtime scope declaration of refaster-compiler.

For me a review of the latest commits and RefasterRuleCompilerTaskListener left.

rickie · 2022-10-03T13:16:20Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

+      Description description, Optional<SeverityLevel> severityOverride) {
+    return Description.builder(
+            description.position,
+            "Refaster Rule",


We use both Refaster Rule and Refaster Template a lot. Not really sure if we need to settle on using one of the two.
Now it depends a bit on the situation, I feel.

rickie · 2022-10-03T13:22:34Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

+      Description description, Optional<SeverityLevel> severityOverride) {
+    return Description.builder(
+            description.position,
+            "Refaster Rule",


We use both Refaster Rule and Refaster Template a lot. Not really sure if we need to settle on using one of the two.
Now it depends a bit on the situation, I feel.

To be honest, I try to only use the word "template" to avoid confusing and make people use the same words. I'd be okay with switching everything to template. WDYT?

Badbond

Another intermittent review for posting the following suggested commit message. Feel free to tweak/suggest an alternative. Slightly different than from 0561c37 given that we have changed more here.

Suggested commit message:

Augment `Description`s of Refaster rule matches (#255)

By emitting a website link, if available, and reporting the matching Refaster
rule in the description's message rather than its check name. Additionally, it
is now possible to assign a custom severity to Refaster rules and to override 
the severity of matches for all Refaster rules by passing
`-Xep:Refaster:[SEVERITY]`.

Badbond · 2022-10-04T11:25:40Z

...support/src/main/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformer.java

+ * A {@link CompositeCodeTransformer} that can be augmented with annotations.
+ *
+ * <p>The content of the annotations can be used for enriched compilation warnings or errors, and
+ * documentation purposes.


Thanks for the Javadocs @rickie! I jotted something down in a private note when I had a sudden brainpulse a while ago:

A composite code transformer that enhances the description of matches from Refaster rules based on optionally available annotations.

Placing it here to see if (part of it) might be nice to still adopt. Note, ^ is more focused around current functionality and Refaster rules specifically. Feel free to consider s/enhances/augments/ and s/rules/templates/.

It's a good one, augmenting the description of matches is indeed the two examples explained in one go. Will push a change :).

While debugging, I got a better understanding of how the annotations are retrieved and passed down. As such, I understood that the annotations available inside this AnnotatedCompositeCodeTransformer are (per annotation type) of the first available parent class (i.e. of one of the encompassing parent template collections) of the RefasterRule CodeTransformers herein. Here, we augment descriptions on matches per respective CodeTransformer based on annotations available on said CodeTransformer and otherwise use the encompassing annotations mentioned above (not that this class knows, it just has been given a list of fallback annotations to rely upon).

As such, I'm wondering whether we should clarify this Javadoc accordingly as 'the annotations' is currently a bit ambigious over which annotations in the context of (Composite) CodeTransformers we are talking about. Committed a suggestion, PTAL. Not entirely happy with 'as a fallback' but was less happy with words such as 'otherwise' or 'default'.

Also not sure if we should mention the annotations in the summary given the class' name, but left it for now. I did change the summary line as semantically it was saying that the annotations could augment this CompositeCodeTransformer. Also removed the XXX for lack of a better alternative on my side too.

Feel free to revert or change if I did not get the design of this class correctly 😅

…-prone-contrib`

Stephan202 · 2022-10-11T16:30:46Z

Rebased; PR targets default branch again.

Stephan202 · 2022-10-11T16:46:06Z

Tweaked suggested commit message (CC @Badbond):

Augment `Description`s of Refaster rule matches (#255)

By emitting a website link, if available, and reporting the matching Refaster
rule in the description's message rather than the check name. Additionally, it
is now possible to provide a custom explanatory text, to assign a custom
severity to Refaster rules, and to override the severity of matches for all
Refaster rules by passing `-Xep:Refaster:[SEVERITY]`.

Violations of Refaster rules defined in this repository now include a link to
the rule's documentation, hosted on https://error-prone.picnic.tech.

Badbond

Thanks @rickie for the rebasing and renaming efforts! 👍
I am considering not applying @OnlineDocumentation on two Refaster rule collections. WDYT?

Badbond · 2022-10-11T17:16:33Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java

@@ -72,6 +73,7 @@
 // XXX: As-is these rules do not result in a complete migration:
 // - Expressions containing comments are skipped due to a limitation of Refaster.
 // - Assertions inside lambda expressions are also skipped. Unclear why.
+@OnlineDocumentation


Assuming these are migration rules, do we wish to keep these enabled by default in the future? Otherwise it might be nice not to add @OnlineDocumentation now where we can filter this one out when we start extracting these classes in docgen.

Hmm, this one is likely to trigger if not explicitly disabled, so perhaps having the link would be nice? (I'm not too worried about breaking the URL later on, TBH.)

Alright. Let's revisit this then when we probably have the functionality to exclude certain collections/specific rules 👍.

Badbond · 2022-10-11T17:31:55Z

...test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java


 /** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */
+@OnlineDocumentation


Are we expecting this to be documented on the website? Otherwise we should remove this annotation, I think.

Nice catch! Will drop.

That said, the script should anyway restrict itself to */main/*, since refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java does need the documentation, but should be ignored regardless :)

Badbond · 2022-10-11T17:36:17Z

Slightly tweaked suggested commit message.

Stephan202

Added a commit. Commit tweak LGTM; reflowed it.

Stephan202 · 2022-10-11T16:36:10Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/CodeTransformersTest.java

@@ -12,6 +12,11 @@ final class CodeTransformersTest {
  @Test


☝️ likely got lost during conflict resolution, but there was also a small tweak here.

Odd; this comment should have been flushed during the previous review round.

Ah yeah there was something there. I think I got it wrong during rebasing haha.

Stephan202 · 2022-10-11T17:40:31Z

...test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java


 /** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */
+@OnlineDocumentation


Nice catch! Will drop.

Stephan202 · 2022-10-11T17:44:18Z

...test-support/src/test/java/tech/picnic/errorprone/refaster/test/MatchInWrongMethodRules.java


 /** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */
+@OnlineDocumentation


That said, the script should anyway restrict itself to */main/*, since refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/FooRules.java does need the documentation, but should be ignored regardless :)

Stephan202 · 2022-10-11T17:45:33Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java

@@ -72,6 +73,7 @@
 // XXX: As-is these rules do not result in a complete migration:
 // - Expressions containing comments are skipped due to a limitation of Refaster.
 // - Assertions inside lambda expressions are also skipped. Unclear why.
+@OnlineDocumentation


Hmm, this one is likely to trigger if not explicitly disabled, so perhaps having the link would be nice? (I'm not too worried about breaking the URL later on, TBH.)

Badbond

Nothing more from my side! Let's go! 🚀

rickie · 2022-10-12T06:53:33Z

I've got one more thing to consider. I think it could be smart to drop the @OnlineDocumentation from the AssertJRules rule collection. We don't have a test file there, so there is nothing to show, other than the name of the templates.

rickie · 2022-10-12T06:55:17Z

Due to connection in the metro I think my previous comment didn't get through;

Latest changes LGTM!

Tweaked the suggested commit message as there was some duplication in there 👀. WDYT?

Stephan202 · 2022-10-12T07:01:37Z

I've got one more thing to consider. I think it could be smart to drop the @OnlineDocumentation from the AssertJRules rule collection. We don't have a test file there, so there is nothing to show, other than the name of the templates.

Works for me 👍

Due to connection in the metro I think my previous comment didn't get through;

Ah, so it was not just me. Connectivity was worse today than generally.

Tweaked the suggested commit message as there was some duplication in there

Tnx; looks like that was my fault.

Badbond · 2022-10-12T07:13:53Z

I've got one more thing to consider. I think it could be smart to drop the @OnlineDocumentation from the AssertJRules rule collection. We don't have a test file there, so there is nothing to show, other than the name of the templates.

Added a commit to drop it.

This was referenced Sep 21, 2022

Update BugPattern#{link,linkType.CUSTOM} to link to our website #251

Merged

Set up documentation website generation and deployment #253

Merged

japborst reviewed Sep 22, 2022

View reviewed changes

Stephan202 commented Sep 22, 2022

View reviewed changes

Stephan202 force-pushed the sschroevers/refaster-custom-urls branch 3 times, most recently from b5d73cb to 3e47909 Compare September 23, 2022 20:01

Stephan202 commented Sep 24, 2022

View reviewed changes

Stephan202 mentioned this pull request Sep 26, 2022

Speed up Refaster bug checker #261

Draft

Stephan202 force-pushed the sschroevers/refaster-custom-urls branch from ac2b91e to 73c5cb3 Compare September 28, 2022 15:03

Stephan202 changed the base branch from master to rossendrijver/move_matchers September 28, 2022 15:03

Base automatically changed from rossendrijver/move_matchers to master September 29, 2022 08:29

rickie added the new feature label Sep 29, 2022

rickie added this to the 0.4.0 milestone Sep 29, 2022

rickie force-pushed the sschroevers/refaster-custom-urls branch from 73c5cb3 to 3f3cf3c Compare September 29, 2022 08:33

rickie reviewed Sep 29, 2022

View reviewed changes

rickie marked this pull request as ready for review September 29, 2022 19:07

rickie reviewed Sep 29, 2022

View reviewed changes

Stephan202 force-pushed the sschroevers/refaster-custom-urls branch from 790638e to 1f4275a Compare September 29, 2022 21:31

Stephan202 commented Sep 29, 2022

View reviewed changes

Stephan202 commented Sep 30, 2022

View reviewed changes

Badbond self-requested a review September 30, 2022 07:32

Badbond reviewed Sep 30, 2022

View reviewed changes

Stephan202 commented Sep 30, 2022

View reviewed changes

rickie reviewed Oct 3, 2022

View reviewed changes

rickie force-pushed the sschroevers/refaster-custom-urls branch from 424d772 to 49f1d00 Compare October 4, 2022 10:45

Badbond reviewed Oct 4, 2022

View reviewed changes

rickie and others added 11 commits October 11, 2022 18:28

Fix typo

1e9253e

Introduce AnnotatedCompositeCodeTransformerTest

d6f8106

Improve test assumption reasoning

e00e4b0

Add missing comma

7c2cbab

Tweak

432c954

Some instructions for our future selves

ec1895b

s/Refaster template/Refaster rule/ in OnlineDocumentation

1c5514e

Sorry forgot the test

54bba33

s/templates/rule/ for some classes, still WIP

74835ef

Make last files consistent with refaster rules

e8af239

Add @OnlineDocumentation to the Refaster rule collections in `error…

ea33a0b

…-prone-contrib`

Stephan202 force-pushed the sschroevers/refaster-custom-urls branch from 2837464 to ea33a0b Compare October 11, 2022 16:29

Tweaks

66398ab

Badbond reviewed Oct 11, 2022

View reviewed changes

Drop it

b07096a

Stephan202 commented Oct 11, 2022

View reviewed changes

Badbond approved these changes Oct 11, 2022

View reviewed changes

Drop @OnlineDocumentation from TestNGToAssertJRules

9f1d286

rickie merged commit d5a7818 into master Oct 12, 2022

rickie deleted the sschroevers/refaster-custom-urls branch October 12, 2022 09:41

rickie added improvement and removed new feature labels Oct 13, 2022

Stephan202 mentioned this pull request Oct 25, 2022

Prefer Flux#take(long, boolean) over Flux#take(long) to limit upstream generation #314

Merged

		@@ -0,0 +1,12 @@
		package tech.picnic.errorprone.refaster.annotation;


		/** Refaster templates related to expressions dealing with {@link Optional}s. */
		@OnlineDocumentation

	checkState(index >= 0, "String '%s' does not contain character '%s'", message, ':');
	checkState(index >= 0, "String '%s' does not contain character ':', message);

		int index = message.indexOf(':');
		checkState(index >= 0, "String '%s' does not contain character ':'", message);

		* VisitorState#reportMatch(Description)} will override the reported severity only if this bug
		* checker's severity was explicitly configured.

	* and the aforementioned flag is configured.
	* and the aforementioned flag is set.


		/** Refaster rule collection to validate reporting of a match occurring in an unexpected place. */
		@OnlineDocumentation

Emit website link along with Refaster refactor suggestions #255

Emit website link along with Refaster refactor suggestions #255

Conversation

Stephan202 commented Sep 21, 2022 • edited by rickie Loading

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Sep 28, 2022

rickie 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

Badbond 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

Badbond Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

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

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond left a comment • edited by Stephan202 Loading

Choose a reason for hiding this comment

Badbond Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Oct 11, 2022

Stephan202 commented Sep 21, 2022 •

edited by rickie

Loading

Badbond Sep 30, 2022 •

edited

Loading

Badbond Sep 30, 2022 •

edited

Loading

Badbond left a comment •

edited by Stephan202

Loading

Badbond Oct 4, 2022 •

edited

Loading

Stephan202 commented Oct 11, 2022 •

edited by rickie

Loading