Add checkstyle plugin #6901

optionsome · 2025-09-25T13:14:13Z

Summary

Introduce checkstyle. This could be introduced alone or preferably with something that fixes most of the issues automatically (such as openrewrite).

I haven't fixed any of the errors yet. Almost all of them can be automatically fixed with OpenRewrite.

Issue

Related to last week's conference discussions and #6913.

Unit tests

No

Documentation

TODO

Changelog

Not sure if needed

application/src/ext-test/java/org/opentripplanner/ext/fares/impl/OrcaFareServiceTest.java

application/src/ext-test/java/org/opentripplanner/ext/flex/FlexIntegrationTestData.java

application/src/ext/java/org/opentripplanner/ext/dataoverlay/GenericDataFile.java

application/src/ext/java/org/opentripplanner/ext/debugrastertiles/EdgeVertexTileRenderer.java

optionsome · 2025-09-25T13:25:48Z

pom.xml

@@ -292,22 +287,54 @@
                </configuration>
            </plugin>

-            <plugin>
-                <groupId>com.diffplug.spotless</groupId>
-                <artifactId>spotless-maven-plugin</artifactId>


I removed spotless because I thought openrewrite could do it's job. We have checks for the imports but do we want to additionally fix the issues with spotless?

Can we please keep the plugin and have it perhaps deactivated by default? I find it super-useful to remove all unused imports with mvn spotless:apply.

Do we really want to still use spotless as well? I tried to limit the number of tools we use since checkstyle/openrewrite check/fix these issues as well. Although, spotless is faster than openrewrite.

pom.xml

leonardehrenfried · 2025-09-26T10:07:42Z

application/src/ext/java/org/opentripplanner/ext/flex/FlexRouter.java

    final List<FlexServiceDate> dates = new ArrayList<>();

    // TODO - This code id not DRY, the same logic is in RaptorRoutingRequestTransitDataCreator
-    for (int d = -additionalPastSearchDays; d <= additionalFutureSearchDays; ++d) {


What is the problem being fixed here?

application/src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLDataFetchers.java

leonardehrenfried

Right now I feel that the code changes in this PR are a bit too invasive. I would start a bit more carefully with a smaller set of changes. I feel that right now that OpenRewrite and Checkstyle don't interact all that well with each other.

Checkstyle

Focusses more on style issues like braces and not initializing two variables on the same line. No automatic fixes.

OpenRewrite

Focusses on directly fixing all issues but the support for style problems is weaker.

Combining them

I feel that trying to combine right now is too much complexity at the same time. I would go with one tool and see how it feels. Personally, I would focus on OpenRewrite but it might not have the support for the style issues that @optionsome cares about.

leonardehrenfried · 2025-09-26T11:33:32Z

Now that I said it, I took another look and there is support for the braces and the double initialization:

            <plugin>
                <groupId>org.openrewrite.maven</groupId>
                <artifactId>rewrite-maven-plugin</artifactId>
                <version>6.18.0</version>
                <configuration>
                    <activeRecipes>
                        <recipe>org.openrewrite.staticanalysis.MultipleVariableDeclarations</recipe>
                        <recipe>org.openrewrite.staticanalysis.NeedBraces</recipe>
                    </activeRecipes>
                </configuration>
                <dependencies>
                    <dependency>
                        <groupId>org.openrewrite.recipe</groupId>
                        <artifactId>rewrite-static-analysis</artifactId>
                        <version>2.18.0</version>
                    </dependency>
                </dependencies>
            </plugin>

With this clarified I would skip checkstyle and put all the effort into openrewrite. We have to enable the recipes bit by bit and figure out when it should be run because it is slower than checkstyle.

optionsome · 2025-09-26T12:24:39Z

I will test it out. We should have a discussion on if we only want rules that can be automatically fixed or not.

optionsome · 2025-09-29T07:20:12Z

I think the only relevant part here anymore is the checkstyle config, which we might partly use.

optionsome · 2025-10-03T20:29:04Z

I cleaned up this pr so it now just includes checkstyle configuration. Almost all of the checkstyle rules can be automatically fixed with openrewrite (which I haven't done here at least yet). One option is that we run checkstyle as normal part of the build (it's fast) and then you have to run openrewrite to fix the issues (there are some issues that openrewrite doesn't fix even though it should). You shouldn't really get these errors often unless you are new to the project and unfamiliar with our code style.

optionsome · 2025-10-03T20:30:50Z

If we decide to use checkstyle, I will add a profile to skip it if similarly as we have for prettier (if it's possible).

optionsome · 2025-10-09T21:23:39Z

application/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/QueryTypeImpl.java

-      List<String> filterByBikeParks = null;
-      List<String> filterByCarParks = null;

+      // TODO implement


Suggested change

// TODO implement

optionsome · 2025-10-09T21:29:32Z

application/src/main/java/org/opentripplanner/routing/services/notes/StreetNotesService.java

    for (StreetNoteModel source : sources) {
      Set<StreetNoteAndMatcher> maas2 = source.getNotes(edge);
-      if (maas2 != null) maas.addAll(maas2);
+      if (maas2 != null) {


Why do we still have street notes? I was under the impression that it was some feature that we already removed.

I checked issue history and seemingly @hbruch was using it some years ago still.

optionsome · 2025-10-09T21:33:53Z

...java/org/opentripplanner/raptor/moduletests/F11_AccessWithRidesMultipleOptimalPathsTest.java

-    // Best duration 18m,
-    var transitAndTransit = startWalkAndL1 + endL2AndWalk + "[0:02 0:23 21m Tₓ1]";
    // Min-Duration 19m
+    // Best duration 18m,


Suggested change

// Best duration 18m,

optionsome · 2025-10-09T21:34:20Z

...java/org/opentripplanner/raptor/moduletests/F11_AccessWithRidesMultipleOptimalPathsTest.java

-    // Min-Duration 19m - this is
-    var flexTransferTransit = startFlexAccess + endWalkAndL3 + "[0:02 0:22 20m Tₓ1 C₁2_580]";
    // Best duration 16m,
+    // Min-Duration 19m - this is


Suggested change

// Min-Duration 19m - this is

optionsome · 2025-10-09T21:36:25Z

pom.xml

@@ -292,22 +287,54 @@
                </configuration>
            </plugin>

-            <plugin>
-                <groupId>com.diffplug.spotless</groupId>
-                <artifactId>spotless-maven-plugin</artifactId>


Do we really want to still use spotless as well? I tried to limit the number of tools we use since checkstyle/openrewrite check/fix these issues as well. Although, spotless is faster than openrewrite.

t2gran · 2025-10-13T16:34:23Z

I would like us to add the configuration without any rules in the initial PR. Then add one rule at the time with the code fixed. If we want to make sure this works adding one rule in this PR is ok, for example AvoidStarImport. The AvoidStarImport only has 2 places witch needs to be fixed - so it is good for first rule to include.

optionsome · 2025-10-21T13:41:44Z

#6988 Replaces this pr. I will keep this open as draft so we can copy over some rules/recipes in follow up prs from here.

optionsome · 2025-10-21T13:44:20Z

I would like us to add the configuration without any rules in the initial PR. Then add one rule at the time with the code fixed. If we want to make sure this works adding one rule in this PR is ok, for example AvoidStarImport. The AvoidStarImport only has 2 places witch needs to be fixed - so it is good for first rule to include.

I feel there would be too much process if we add the rules one by one in prs. So I grouped the import related rules in #6988 and it's easy to review as the imports either work or don't work.

leonardehrenfried · 2025-11-11T09:09:24Z

I think this can be closed

optionsome · 2025-11-11T10:21:04Z

I think this can be closed

I was planning to close this after I've added in separate prs the stuff that is in this pr.

optionsome requested a review from a team as a code owner September 25, 2025 13:14

optionsome added the !Technical Debt Improve code quality, no functional changes. label Sep 25, 2025

optionsome commented Sep 25, 2025

View reviewed changes

leonardehrenfried reviewed Sep 26, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLDataFetchers.java Show resolved Hide resolved

leonardehrenfried reviewed Sep 26, 2025

View reviewed changes

optionsome mentioned this pull request Sep 26, 2025

Replace spotless with openrewrite and add more stylistic/cleanup rules #6907

Closed

optionsome marked this pull request as draft September 29, 2025 07:20

optionsome force-pushed the checkstyle branch from 7f606ec to ddc0a14 Compare October 3, 2025 20:25

optionsome changed the title ~~Add checkstyle and run automatic code clean up with openrewrite~~ Add checkstyle plugin Oct 3, 2025

optionsome mentioned this pull request Oct 3, 2025

Tools for improving OTP's quality and development process #6913

Closed

optionsome added 5 commits October 9, 2025 23:20

Replace spotless with checkstyle

fe0bacc

Add option to skip checkstyle

b546f46

Add openrewrite to automatically fix code style issues

7203b41

Update docs

d54e0c3

Run OpenRewrite to fix some code style issues

6c14fc1

optionsome force-pushed the checkstyle branch from ddc0a14 to 6c14fc1 Compare October 9, 2025 21:21

optionsome commented Oct 9, 2025

View reviewed changes

t2gran added this to the 2.9 (next release) milestone Nov 19, 2025

Add checkstyle plugin #6901

Are you sure you want to change the base?

Add checkstyle plugin #6901

Conversation

optionsome commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Changelog

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Checkstyle

OpenRewrite

Combining them

Uh oh!

leonardehrenfried commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

optionsome commented Sep 26, 2025

Uh oh!

optionsome commented Sep 29, 2025

Uh oh!

optionsome commented Oct 3, 2025

Uh oh!

optionsome commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t2gran commented Oct 13, 2025

Uh oh!

optionsome commented Oct 21, 2025

Uh oh!

optionsome commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonardehrenfried commented Nov 11, 2025

Uh oh!

optionsome commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

optionsome commented Sep 25, 2025 •

edited

Loading

leonardehrenfried commented Sep 26, 2025 •

edited

Loading

optionsome commented Oct 21, 2025 •

edited

Loading