Skip to content

Improve walk step narrative for entering/exiting stations and signposted pathways#5285

Merged
leonardehrenfried merged 13 commits into
opentripplanner:dev-2.xfrom
ibi-group:pathway-steps
Aug 17, 2023
Merged

Improve walk step narrative for entering/exiting stations and signposted pathways#5285
leonardehrenfried merged 13 commits into
opentripplanner:dev-2.xfrom
ibi-group:pathway-steps

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Aug 11, 2023

Copy link
Copy Markdown
Member

Summary

This PR adds 3 three new relative direction instructions: "enter station", "exit station" and "follow signs". The information for it is taken from the GTFS data.

Refactoring

The following refactorings are also in this PR:

  • the walk step was made completely immutable and a builder was introduced
  • the "name" property of the Pathway was renamed to signpostedAs to better reflect the difference between those two concepts.

Unit tests

Added.

Documentation

Javadoc.

@leonardehrenfried leonardehrenfried added !Improvement A functional improvement or micro feature !Technical Debt Improve code quality, no functional changes. IBI Developed by or important for IBI Group +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels Aug 11, 2023
@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner August 11, 2023 15:58
@codecov

codecov Bot commented Aug 11, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 91.45% and project coverage change: +0.07% 🎉

Comparison is base (503ac5e) 65.99% compared to head (1a8ad58) 66.07%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5285      +/-   ##
=============================================
+ Coverage      65.99%   66.07%   +0.07%     
- Complexity     14890    14943      +53     
=============================================
  Files           1778     1780       +2     
  Lines          68912    68969      +57     
  Branches        7286     7287       +1     
=============================================
+ Hits           45481    45572      +91     
+ Misses         20946    20914      -32     
+ Partials        2485     2483       -2     
Files Changed Coverage Δ
...org/opentripplanner/street/search/state/State.java 81.81% <ø> (ø)
...entripplanner/street/search/state/StateEditor.java 77.71% <ø> (-0.13%) ⬇️
...builder/module/AddTransitModelEntitiesToGraph.java 59.28% <33.33%> (+2.05%) ⬆️
...er/ext/gtfsgraphqlapi/mapping/DirectionMapper.java 88.88% <88.88%> (ø)
...ing/algorithm/mapping/StatesToWalkStepsMapper.java 82.93% <90.00%> (+1.79%) ⬆️
...rg/opentripplanner/model/plan/WalkStepBuilder.java 92.72% <92.72%> (ø)
...ipplanner/api/mapping/RelativeDirectionMapper.java 80.00% <94.11%> (+7.77%) ⬆️
...nner/ext/gtfsgraphqlapi/datafetchers/stepImpl.java 76.19% <100.00%> (+29.38%) ⬆️
...ner/ext/gtfsgraphqlapi/generated/GraphQLTypes.java 9.35% <100.00%> (+0.18%) ⬆️
...pentripplanner/api/model/ApiRelativeDirection.java 100.00% <100.00%> (ø)
... and 9 more

... and 5 files with indirect coverage changes

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

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Results are as expected, code looks good at first glance too! If I have proper review access I can leave a more detailed review

optionsome
optionsome previously approved these changes Aug 15, 2023
public Edge backEdge;

// allow traverse result chaining (multiple results)
protected State next;

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.

We have had an unused field in the state?

@leonardehrenfried leonardehrenfried Aug 15, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I removed the accessor methods in #4841 but never the actual field. 😱

vpaturet
vpaturet previously approved these changes Aug 16, 2023
@leonardehrenfried

Copy link
Copy Markdown
Member Author

I had merge dev-2.x because of a conflict with #5267.

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

Labels

+Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR IBI Developed by or important for IBI Group !Improvement A functional improvement or micro feature !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants