Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename StopModel to SiteRepository #6165

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Rename StopModel to SiteRepository #6165

merged 2 commits into from
Oct 22, 2024

Conversation

vpaturet
Copy link
Contributor

Summary

Rename:
StopModel --> SiteRepository
StopModelIndex --> SiteRepositoryIndex

The more generic term of Site is preferred over Stop.
The TransModel definition of a SITE is:

A well known PLACE to which passengers may refer to
indicate the origin or a destination of a trip.

Issue

No

Unit tests

No

Documentation

No

@vpaturet vpaturet added Technical Debt bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR labels Oct 16, 2024
@vpaturet vpaturet self-assigned this Oct 16, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.62016% with 25 lines in your changes missing coverage. Please review.

Project coverage is 69.93%. Comparing base (1e3ffe0) to head (029d96c).
Report is 78 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...org/opentripplanner/netex/mapping/NetexMapper.java 50.00% 7 Missing ⚠️
...planner/transit/service/DefaultTransitService.java 53.33% 7 Missing ⚠️
...ipplanner/model/impl/OtpTransitServiceBuilder.java 55.55% 4 Missing ⚠️
...ripplanner/ext/flex/AreaStopsToVerticesMapper.java 75.00% 0 Missing and 1 partial ⚠️
...r/ext/transferanalyzer/DirectTransferAnalyzer.java 0.00% 1 Missing ⚠️
...rg/opentripplanner/graph_builder/GraphBuilder.java 0.00% 1 Missing ⚠️
...der/module/RouteToCentroidStationIdsValidator.java 0.00% 1 Missing ⚠️
.../module/geometry/CalculateWorldEnvelopeModule.java 0.00% 1 Missing ⚠️
.../algorithm/raptoradapter/transit/TransitLayer.java 66.66% 0 Missing and 1 partial ⚠️
...n/java/org/opentripplanner/standalone/OTPMain.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##             dev-2.x    #6165      +/-   ##
=============================================
+ Coverage      69.89%   69.93%   +0.03%     
- Complexity     17705    17720      +15     
=============================================
  Files           1996     1996              
  Lines          75334    75420      +86     
  Branches        7713     7717       +4     
=============================================
+ Hits           52657    52743      +86     
- Misses         19998    19999       +1     
+ Partials        2679     2678       -1

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

@vpaturet vpaturet marked this pull request as ready for review October 16, 2024 13:27
@vpaturet vpaturet requested a review from a team as a code owner October 16, 2024 13:27
t2gran
t2gran previously approved these changes Oct 17, 2024
View reviewed changes
habrahamsson-skanetrafiken
habrahamsson-skanetrafiken previously approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw a few names in the tests that were named "model" something. These should probably be renamed now. Most of them weren't related to the stop model though.

But I think you can merge this and do that in a follow-up PR if you don't want to keep this open too long and risk merge conflicts.

...ication/src/test/java/org/opentripplanner/apis/transmodel/mapping/TripRequestMapperTest.java
@@ -89,14 +89,14 @@ public class TripRequestMapperTest implements PlanTestConstants {
.bus(route2, 2, time("11:20"), time("11:40"), Place.forStop(stop3))
.build();
var patterns = itineraryPatterns(itinerary);
var stopModel = TEST_MODEL
.stopModelBuilder()
var siteRepository = TEST_MODEL
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably find a better name for TEST_MODEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming TransitModelForTest to TimetableRepositoryForTest was a mistake in #6148
TransitModelForTest contains test helpers to create timetable and stop entities, it has nothing to do with the repository.
Since it is under org.opentripplanner.transit.model, TransitModelForTest is maybe not such a bad name.
To be fixed in another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the original name wasn't very good either. It should probably be called something with "Factory" or "Helper" imo.

Copy link
Member

Choose a reason for hiding this comment

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

TimetableTestDataFactory ?

application/src/test/java/org/opentripplanner/netex/NetexEpipBundleSmokeTest.java Show resolved Hide resolved
application/src/test/java/org/opentripplanner/netex/NetexNordicBundleSmokeTest.java Show resolved Hide resolved
application/src/test/java/org/opentripplanner/routing/algorithm/GraphRoutingTest.java Show resolved Hide resolved
application/src/test/java/org/opentripplanner/transit/model/_data/PatternTestModel.java Show resolved Hide resolved
application/src/test/java/org/opentripplanner/updater/trip/RealtimeTestConstants.java Outdated Show resolved Hide resolved
@vpaturet
Copy link
Contributor Author

@habrahamsson-skanetrafiken I applied one review suggestion and deferred the other ones to follow-up PRs

@t2gran
Copy link
Member

t2gran commented Oct 21, 2024

Tema for Dev Meeting: When we have massive renaming PRs like this I prefer them to only do one thing. It makes the review much faster. The comments from Henrik on renaming other stuff is good, but we have a lot of these - should we collect them somewhere instead of requesting the author to change them?

@vpaturet vpaturet merged commit 06d969d into opentripplanner:dev-2.x Oct 22, 2024
5 checks passed
t2gran pushed a commit that referenced this pull request Oct 22, 2024
Add changelog entry for #6165 [ci skip]
3b47a09
t2gran pushed a commit that referenced this pull request Oct 22, 2024
Bump serialization version id for #6165
d9b413c
@t2gran t2gran added this to the 2.7 (next release) milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken habrahamsson-skanetrafiken approved these changes

@t2gran t2gran t2gran approved these changes

Assignees

@vpaturet vpaturet

Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Technical Debt
Projects
None yet
Milestone
2.7 (next release)
Development

Successfully merging this pull request may close these issues.
3 participants
@vpaturet @t2gran @habrahamsson-skanetrafiken