Enable the `BugPatternNaming` check #86

Stephan202 · 2022-04-25T16:52:01Z

~~❗ This PR is on top of #76. ❗~~

Open question: should we introduce a new subpackage for non-BugChecker utility classes, so that it's easier to keep an overview? Or should we organize the classes some other way?

Suggested commit message (unless we introduce additional changes):

Enable the `BugPatternNaming` check (#86)

Stephan202 · 2022-08-01T19:12:42Z

Rebased, resolved conflicts and applied the bug pattern to two new checks introduced since the PR was opened.

Stephan202 · 2022-08-01T19:13:13Z

Open question: should we introduce a new subpackage for non-BugChecker utility classes, so that it's easier to keep an overview? Or should we organize the classes some other way?

Thoughts/opinions? :)

rickie · 2022-08-02T09:54:18Z

Open question: should we introduce a new subpackage for non-BugChecker utility classes, so that it's easier to keep an overview? Or should we organize the classes some other way?

That sounds like a good idea. Perhaps we can (re-)use parts of the structure from EP itself. For example a matchers and util subpackage.

OTOH I think a simple split between BugCheckers and the rest is fine for now. WDYT?

Pushed a commit and rebased.

rickie · 2022-08-02T09:12:32Z

...ne-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableMapTemplates.java

@@ -192,7 +192,7 @@ ImmutableMap<K, V> after(

    // XXX: Instead of `Map.Entry::getKey` we could also match `e -> e.getKey()`. But for some
    // reason Refaster doesn't handle that case. This doesn't matter if we roll out use of
-    // `MethodReferenceUsageCheck`. Same observation applies to a lot of other Refaster checks.
+    // `MethodReferenceUsageC`. Same observation applies to a lot of other Refaster checks.


Commenting to make sure I don't miss this.

rickie · 2022-08-02T09:15:01Z

...prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExplicitEnumOrderingTest.java

  private final CompilationTestHelper compilationTestHelper =
-      CompilationTestHelper.newInstance(ExplicitEnumOrderingCheck.class, getClass());
+      CompilationTestHelper.newInstance(ExplicitEnumOrdering.class, getClass());

  @Test
  void Identification() {


Let's sneak in this small rename.

Suggested change

void Identification() {

void identification() {

Stephan202 · 2022-08-04T10:04:18Z

Rebased, resolved conflicts and added a commit to fix the build.

Stephan202 · 2022-08-04T11:31:00Z

Open question: should we introduce a new subpackage for non-BugChecker utility classes, so that it's easier to keep an overview? Or should we organize the classes some other way?

As discussed offline, I moved the non-BugChecker classes to a .util subpackage. Did require me to document now-public methods. The PR is now ready for review.

rickie

Looks really nice :party-blob:!

rickie · 2022-08-04T12:28:14Z

Suggested commit message:

Enable the `BugPatternNaming` check (#86)

While there, introduce a `.util` subpackage for non-`Bugchecker` classes.

Or use something of this alternative:

Introduce a `.util` subpackage in `errorprone.bugpatterns` for non-`Bugchecker` classes.

Stephan202 · 2022-08-04T12:48:49Z

Would go for the first option 👍

Badbond

This is very satisfying. Got two comments regarding class-naming/JavaDoc. PTAL :) First alternative suggested commit message LGTM (tweaked one word).

Badbond · 2022-08-04T13:47:01Z

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

 // XXX: Can we locate this code in a better place? Maybe contribute it upstream?
-final class Util {
+public final class Util {


Hmm. A util.Util class doesn't look too great. In absence of much different functionality, should we for now make this something like TreeToStringConverter with appropriate class JavaDoc?

The operation is a static method, so and active word like "Converter" is not appropriate IMHO. Maybe Source or SourceCode? 🤔

Changed it to SourceCode and updated the class' JavaDoc. Feel free to revert or tweak 👍

Badbond · 2022-08-04T13:50:54Z

...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MethodMatcherFactory.java

 /** A method invocation expression {@link Matcher} factory. */
 // XXX: Document better. The expressions accepted here could also be defined using `MethodMatchers`.
 // So explain why this class is still useful.


With moving these to a util subpackage and adding JavaDoc to them, I think fixing this XXX now is in order.

In lack of better ideas:

Suggested change

/** A method invocation expression {@link Matcher} factory. */

// XXX: Document better. The expressions accepted here could also be defined using `MethodMatchers`.

// So explain why this class is still useful.

/** A factory for creating resuable Matchers. */

Also, this class could be fully static and be more like a Util class, right?

Right, but I would like to keep this whole class exactly as-is: I have the feeling that this code is all-around very dodgy, and both things (the XXX comment and the could-be-static observation make that clear). Improving this is out of scope.

rickie · 2022-08-05T13:03:39Z

Changes LGTM! Nice BugPattern and some good improvements to our setup 😄.

Stephan202 · 2022-08-07T15:51:19Z

Rebased and added a tiny tweak. Will merge once built.

Stephan202 mentioned this pull request Apr 25, 2022

Upgrade Error Prone 2.10.0 -> 2.14.0 and errorprone-slf4j 0.1.4 -> 0.1.12 #76

Merged

1 task

rickie mentioned this pull request Apr 28, 2022

RequestMappingAnnotation: exempt ServerWebExchange #91

Merged

rickie self-requested a review May 16, 2022 08:37

Stephan202 force-pushed the renovate/version.error-prone-orig branch 2 times, most recently from 4b51792 to 4dcec2a Compare June 6, 2022 07:13

rickie force-pushed the renovate/version.error-prone-orig branch from 4dcec2a to 5887bbf Compare June 17, 2022 17:39

rickie force-pushed the renovate/version.error-prone-orig branch from 5887bbf to 5bb5066 Compare July 6, 2022 11:52

Stephan202 force-pushed the renovate/version.error-prone-orig branch from 568bde6 to 9704cb2 Compare July 31, 2022 12:34

Base automatically changed from renovate/version.error-prone-orig to master July 31, 2022 13:50

Stephan202 force-pushed the sschroevers/enable-BugPatternNaming-check branch from 1b187dd to 8a89388 Compare August 1, 2022 19:12

rickie force-pushed the sschroevers/enable-BugPatternNaming-check branch from 8a89388 to 0feb610 Compare August 2, 2022 09:54

rickie reviewed Aug 2, 2022

View reviewed changes

Stephan202 force-pushed the sschroevers/enable-BugPatternNaming-check branch from 0feb610 to 002354f Compare August 4, 2022 10:03

Stephan202 marked this pull request as ready for review August 4, 2022 11:31

Stephan202 requested a review from Badbond August 4, 2022 11:31

Stephan202 added this to the 0.1.0 milestone Aug 4, 2022

rickie approved these changes Aug 4, 2022

View reviewed changes

Badbond reviewed Aug 4, 2022

View reviewed changes

Badbond approved these changes Aug 5, 2022

View reviewed changes

Stephan202 and others added 3 commits August 7, 2022 17:48

Enable the BugPatternNaming check

1dd6c51

Tweaks

ad55356

Post-rebase fixes

bb0ea81

Stephan202 and others added 3 commits August 7, 2022 17:48

Move utility classes to subpackage

2f17dca

Rename class Util -> SourceCode

c752c80

Tweak

2886734

Stephan202 force-pushed the sschroevers/enable-BugPatternNaming-check branch from ff6c7e3 to 2886734 Compare August 7, 2022 15:50

Stephan202 merged commit 21646ff into master Aug 7, 2022

Stephan202 deleted the sschroevers/enable-BugPatternNaming-check branch August 7, 2022 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the `BugPatternNaming` check #86

Enable the `BugPatternNaming` check #86

Stephan202 commented Apr 25, 2022 •

edited

Loading

Stephan202 commented Aug 1, 2022

Stephan202 commented Aug 1, 2022

rickie commented Aug 2, 2022 •

edited

Loading

rickie Aug 2, 2022

rickie Aug 2, 2022

Stephan202 commented Aug 4, 2022

Stephan202 commented Aug 4, 2022

rickie left a comment

rickie commented Aug 4, 2022 •

edited by Badbond

Loading

Stephan202 commented Aug 4, 2022

Badbond left a comment

Badbond Aug 4, 2022

Stephan202 Aug 4, 2022

Badbond Aug 5, 2022

Badbond Aug 4, 2022 •

edited

Loading

Stephan202 Aug 4, 2022

rickie commented Aug 5, 2022

Stephan202 commented Aug 7, 2022

Enable the BugPatternNaming check #86

Enable the BugPatternNaming check #86

Conversation

Stephan202 commented Apr 25, 2022 • edited Loading

Stephan202 commented Aug 1, 2022

Stephan202 commented Aug 1, 2022

rickie commented Aug 2, 2022 • edited Loading

rickie Aug 2, 2022

Choose a reason for hiding this comment

rickie Aug 2, 2022

Choose a reason for hiding this comment

Stephan202 commented Aug 4, 2022

Stephan202 commented Aug 4, 2022

rickie left a comment

Choose a reason for hiding this comment

rickie commented Aug 4, 2022 • edited by Badbond Loading

Stephan202 commented Aug 4, 2022

Badbond left a comment

Choose a reason for hiding this comment

Badbond Aug 4, 2022

Choose a reason for hiding this comment

Stephan202 Aug 4, 2022

Choose a reason for hiding this comment

Badbond Aug 5, 2022

Choose a reason for hiding this comment

Badbond Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Stephan202 Aug 4, 2022

Choose a reason for hiding this comment

rickie commented Aug 5, 2022

Stephan202 commented Aug 7, 2022

Enable the `BugPatternNaming` check #86

Enable the `BugPatternNaming` check #86

Stephan202 commented Apr 25, 2022 •

edited

Loading

rickie commented Aug 2, 2022 •

edited

Loading

rickie commented Aug 4, 2022 •

edited by Badbond

Loading

Badbond Aug 4, 2022 •

edited

Loading