Skip to content

Conversation

optionsome
Copy link
Member

@optionsome optionsome commented Sep 26, 2025

Summary

This is an alternative to #6901. Might also integrate checkstyle in a separate pr.

Issue

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

Unit tests

No

Documentation

TODO

Changelog

Not sure

@optionsome optionsome requested a review from a team as a code owner September 26, 2025 15:03
Copy link

codecov bot commented Sep 26, 2025

Codecov Report

❌ Patch coverage is 35.90226% with 341 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.99%. Comparing base (53cd688) to head (32007fc).
⚠️ Report is 1 commits behind head on dev-2.x.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...java/org/opentripplanner/visualizer/ShowGraph.java 0.00% 17 Missing ⚠️
...ation/src/main/java/com/jhlabs/awt/TextStroke.java 0.00% 16 Missing ⚠️
...va/org/opentripplanner/apis/gtfs/GraphQLUtils.java 0.00% 8 Missing and 2 partials ⚠️
.../opentripplanner/gtfs/graphbuilder/GtfsModule.java 9.09% 8 Missing and 2 partials ⚠️
...tripplanner/ext/fares/impl/AtlantaFareService.java 0.00% 4 Missing and 4 partials ⚠️
.../java/org/opentripplanner/astar/model/BinHeap.java 55.55% 4 Missing and 4 partials ⚠️
.../opentripplanner/raptor/util/CompareIntArrays.java 0.00% 8 Missing ⚠️
...grastertiles/TraversalPermissionsEdgeRenderer.java 0.00% 6 Missing ⚠️
...tion/src/main/java/com/jhlabs/awt/ShapeStroke.java 0.00% 6 Missing ⚠️
...entripplanner/model/plan/leg/ElevationProfile.java 25.00% 3 Missing and 3 partials ⚠️
... and 89 more
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6907      +/-   ##
=============================================
- Coverage      72.10%   71.99%   -0.12%     
+ Complexity     19671    19670       -1     
=============================================
  Files           2125     2125              
  Lines          79533    79769     +236     
  Branches        8051     8052       +1     
=============================================
+ Hits           57349    57428      +79     
- Misses         19347    19504     +157     
  Partials        2837     2837              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@optionsome optionsome force-pushed the openrewrite branch 2 times, most recently from 5ab59a9 to 6cfe91c Compare September 29, 2025 10:20
@optionsome
Copy link
Member Author

I ran checkstyle locally to test if openrewrite fixed all the issues it was supposed(?) to fix and it found some additional issues but not too many:

[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/osm/model/OsmEntity.java:566:7: 'for' construct must use '{}'s. [NeedBraces]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/osm/model/OsmEntity.java:591:5: 'for' construct must use '{}'s. [NeedBraces]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/osm/model/OsmEntity.java:593:5: 'for' construct must use '{}'s. [NeedBraces]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/updater/vehicle_rental/datasources/gbfs/v3/GbfsStationInformationMapper.java:8:54: Using the '.*' form of import should be avoided - org.mobilitydata.gbfs.v3_0.station_information.*. [AvoidStarImport]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/graph_builder/GraphStats.java:161:7: 'for' construct must use '{}'s. [NeedBraces]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java:181:5: 'for' construct must use '{}'s. [NeedBraces]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/visualizer/ShowGraph.java:615:7: Unused local variable 'mode'. [UnusedLocalVariable]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchemaFactory.java:900:13: Unused local variable 'filterByBikeParks'. [UnusedLocalVariable]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchemaFactory.java:901:13: Unused local variable 'filterByCarParks'. [UnusedLocalVariable]
[ERROR] /home/joel/OpenTripPlanner/application/src/main/java/org/opentripplanner/framework/geometry/GeometryUtils.java:223:9: 'for' construct must use '{}'s. [NeedBraces]

@optionsome optionsome marked this pull request as draft September 30, 2025 09:43
```

To check for formatting errors, use the profile `prettierCheck`:
To check for prettier formatting errors, use the profile `prettierCheck`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier and openrewrite both do formatting changes, that's why I wanted to specify that this command only runs some of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't realize the title of the section. I've now split the rewrite stuff to be under different title.

Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked fast through this. The changes are mostly inline with existing OTP conventions - very good.

If we want to run this on a permanent bases is something else, I support optIn and just a warning in the build. That allows us to run this periodically, and not on every commit in our local environment.

@optionsome
Copy link
Member Author

I updated this pr so that openrewrite is not run by default (although it is still checked by CI). I also made the CI check simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants