Skip to content

Move REST API into sandbox#5580

Merged
leonardehrenfried merged 13 commits into
opentripplanner:dev-2.xfrom
ibi-group:rest-sandbox
Jan 11, 2024
Merged

Move REST API into sandbox#5580
leonardehrenfried merged 13 commits into
opentripplanner:dev-2.xfrom
ibi-group:rest-sandbox

Conversation

@leonardehrenfried

Copy link
Copy Markdown
Member

Summary

This moves the REST API classes into a sandbox folder and adds a feature switch LegacyRestApi for them. For now this switch will be on. As soon as the new debug UI is ready for prime time, I plan to make it default to off.

Then I suggest we have one more release with the API in a sandbox and remove it afterwards.

Issue

#4828

Documentation

The question is if we want to have Sandbox documentation for this feature. I would say no, since that would actually improve the REST API docs, which we don't want.

@leonardehrenfried leonardehrenfried added the +Sandbox This will be implemented as a Sandbox feature label Dec 18, 2023
@leonardehrenfried leonardehrenfried requested a review from a team as a code owner December 18, 2023 11:27
@codecov

codecov Bot commented Dec 18, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (faeb260) 67.46% compared to head (cdc09ca) 67.46%.
Report is 12 commits behind head on dev-2.x.

Files Patch % Lines
...in/java/org/opentripplanner/apis/APIEndpoints.java 0.00% 4 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5580      +/-   ##
=============================================
- Coverage      67.46%   67.46%   -0.01%     
+ Complexity     16197    16195       -2     
=============================================
  Files           1862     1862              
  Lines          71206    71222      +16     
  Branches        7405     7407       +2     
=============================================
+ Hits           48041    48049       +8     
- Misses         20674    20680       +6     
- Partials        2491     2493       +2     

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

@t2gran t2gran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here is a few package suggestions. The main principle is to organise packages in a logical order without cyclic dependencies. So, to place something in the right package you can look at the usage - where is it used and what does it use. But, currently it is a mess - the bigest problem is that things are organized by type, not by feature. For example all classes involved in paging should be under the top level package o.o.paging, then sub-domain (itinerary-filter and token-generation), then type: model, service, support, mapping and so on. But, I don´t want some code to follow one way and other code to follow another - so at some point we will try to clean up everything (at least move critical mass 80% of the code).

Comment thread src/main/java/org/opentripplanner/apis/common/json/FeedScopedIdDeserializer.java Outdated
Comment thread src/main/java/org/opentripplanner/apis/common/json/FeedScopedIdSerializer.java Outdated
Comment thread src/main/java/org/opentripplanner/apis/common/json/JSONObjectMapperProvider.java Outdated
Comment thread src/main/java/org/opentripplanner/apis/common/json/GraphQLResponseSerializer.java Outdated
Comment thread src/main/java/org/opentripplanner/apis/common/TileJson.java Outdated
Comment thread src/main/java/org/opentripplanner/apis/common/SemanticHash.java Outdated
Comment thread src/main/java/org/opentripplanner/framework/application/OTPFeature.java Outdated
@leonardehrenfried

Copy link
Copy Markdown
Member Author

Ah, sorry Thomas. I accidentally rebased and probably wiped your review state.

@t2gran t2gran self-requested a review January 4, 2024 14:25
Comment thread src/test/java/org/opentripplanner/api/mapping/FareMapperTest.java Outdated
Comment thread src/test/java/org/opentripplanner/api/model/ApiApiTravelOptionsMakerTest.java Outdated
Comment thread src/test/java/org/opentripplanner/api/model/ApiWalkStepTest.java Outdated
Comment thread src/test/java/org/opentripplanner/api/mapping/EnumMapperTest.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/error/PathNotFoundException.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/error/PathNotFoundException.java Outdated
Comment thread src/main/java/org/opentripplanner/apis/gtfs/mapping/StreetNoteMapper.java Outdated
Comment thread src/main/java/org/opentripplanner/apis/APIEndpoints.java Outdated
@t2gran t2gran added this to the 2.5 (next release) milestone Jan 9, 2024
@t2gran

t2gran commented Jan 9, 2024

Copy link
Copy Markdown
Member

The files in src/ext/java/org/opentripplanner/ext/restapi/model/serverinfo should not be moved, they show up in the review without any changes.

@leonardehrenfried

Copy link
Copy Markdown
Member Author

I've moved them back into the main code.

@leonardehrenfried leonardehrenfried merged commit 22ec01e into opentripplanner:dev-2.x Jan 11, 2024
@leonardehrenfried leonardehrenfried deleted the rest-sandbox branch January 11, 2024 07:03
t2gran pushed a commit that referenced this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Sandbox This will be implemented as a Sandbox feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants