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.

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

Allow multiple FlexibleAreas in a FlexibleStopPlace #4922

Merged
merged 12 commits into from Jun 23, 2023

Conversation

hannesj
Copy link
Contributor

@hannesj hannesj commented Mar 1, 2023

Summary

Currently only the first FlexibleArea in a NeTEX FlexibleStopPlace is parsed. Modify the code to create a GrupStop if multiple areas exist.

@hannesj hannesj added this to the 2.3 milestone Mar 1, 2023
@hannesj hannesj marked this pull request as ready for review March 1, 2023 17:50
@hannesj hannesj requested a review from a team as a code owner March 1, 2023 17:50
@t2gran
Copy link
Member

t2gran commented Mar 3, 2023

I asked @hannesj if we(me or @vpaturet) should take over this one. I agree with the comments from @jtorin.

@hannesj
Copy link
Contributor Author

hannesj commented Mar 6, 2023

I addressed most of the comments. There is an open question about the location of the type key (see comment).

@hannesj hannesj requested a review from jtorin March 6, 2023 19:22
@leonardehrenfried
Copy link
Member

Could you please fix the tests?

@hannesj
Copy link
Contributor Author

hannesj commented Mar 7, 2023

Could you please fix the tests?

No, as the tests should fail with the current behaviour and data. We need to clarify the #4922 (comment) above

Copy link
Contributor

@vpaturet vpaturet left a comment

Choose a reason for hiding this comment

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

As suggested by @testower, the key/value pairs should be looked up both at the FlexibleStopPlace level and FlexibleArea level, with precedence at the FlexibleArea level.

@leonardehrenfried leonardehrenfried marked this pull request as draft March 23, 2023 14:41
@t2gran t2gran modified the milestones: 2.3, 2.4 Apr 24, 2023
@testower testower self-assigned this Jun 16, 2023
@testower testower marked this pull request as ready for review June 16, 2023 11:06
@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Patch coverage: 72.41% and project coverage change: +0.06 🎉

Comparison is base (3f68cb7) 65.34% compared to head (47290c4) 65.41%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #4922      +/-   ##
=============================================
+ Coverage      65.34%   65.41%   +0.06%     
- Complexity     14455    14494      +39     
=============================================
  Files           1755     1754       -1     
  Lines          67978    68029      +51     
  Branches        7231     7243      +12     
=============================================
+ Hits           44420    44500      +80     
+ Misses         21091    21063      -28     
+ Partials        2467     2466       -1     
Impacted Files Coverage Δ
...org/opentripplanner/netex/mapping/NetexMapper.java 80.97% <0.00%> (-1.34%) ⬇️
...pentripplanner/transit/model/site/RegularStop.java 89.74% <ø> (ø)
...opentripplanner/netex/mapping/FlexStopsMapper.java 86.48% <79.16%> (+0.77%) ⬆️
...tripplanner/ext/transmodelapi/model/EnumTypes.java 99.27% <100.00%> (+<0.01%) ⬆️
...tripplanner/netex/mapping/TransportModeMapper.java 70.00% <100.00%> (+18.71%) ⬆️

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leonardehrenfried
Copy link
Member

@testower Can you please click "re-request review" when you think that it's time for it?

@testower
Copy link
Contributor

@testower Can you please click "re-request review" when you think that it's time for it?

Yep will do, there's a couple of outstanding issues that I'm trying to resolve first.

@testower
Copy link
Contributor

I believe I have adressed all outstanding issues. I will create a test dataset based on real data and verify manually before asking for a re-review.

@testower
Copy link
Contributor

I have verified this locally with a semi-fictitious dataset (I have excluded the regular stops from this zip, otherwise GitHub won't let me upload it):

test.zip

The dataset contains a flexible service that uses a single flexible stop place, with two areas. One of the areas is of the type UnrestrictedPublicTransportArea. Here is screenshot from the debug client, show an itinerary from a regular stop within one area to a street edge in the other area:

Screenshot 2023-06-21 at 21 56 30

@testower
Copy link
Contributor

testower commented Jun 21, 2023

Should be good to go now @leonardehrenfried (I mean, good to go for another review :) )

@leonardehrenfried
Copy link
Member

Ok, I see that you've already have reviewers (of which I'm not one, because I don't use Netex :)).

@leonardehrenfried leonardehrenfried requested review from leonardehrenfried and removed request for testower and t2gran June 22, 2023 13:28
@vpaturet vpaturet added the Entur test This is currently being tested at Entur label Jun 23, 2023
Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

@testower Do you have merge permission?

@leonardehrenfried leonardehrenfried removed the request for review from jtorin June 23, 2023 07:44
@leonardehrenfried leonardehrenfried dismissed jtorin’s stale review June 23, 2023 07:44

We have other reviewers

@vpaturet vpaturet merged commit 21a25ab into opentripplanner:dev-2.x Jun 23, 2023
5 checks passed
t2gran pushed a commit that referenced this pull request Jun 23, 2023
@testower testower deleted the otp2_support_flex_groups branch June 23, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Entur test This is currently being tested at Entur improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants