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

provider: Fix a contradiction about the route localization #505

Conversation

vperron
Copy link

@vperron vperron commented May 22, 2020

Explain pull request
I understand that choosing which option is still an ongoing discussion, in #491 for instance.

Nevertheless, for now the current specification is contradictory:

  • routes must be within city limits at some point
  • route may not be within city limits 10 lines further

This PR enforces the most restrictive access for now: the route should be within a city boundary, as stated higher up in the document.

Is this a breaking change
Not really, since the specification is currently contradictory, it can't be implemented.

Which spec(s) will this pull request impact?
Provider

Enforce the most restrictive access: the route should be within a city
boundary, as stated higher up in the document.
@vperron vperron requested a review from a team as a code owner May 22, 2020 14:39
@vperron vperron requested a review from a team May 22, 2020 14:39
@thekaveman
Copy link
Collaborator

It's not clear where you are reading the contradiction? Are you referring to this line:

Unless stated otherwise by the municipality, the trips endpoint must return all trips with a route which intersects with the municipality boundary.

This line speaks to which routes must be included (all routes that have any intersection with the municipality boundary). The line changed in this PR speaks to which points must be included in a route (all points, both inside and outside the boundary).

This change does not look correct to me.

@thekaveman thekaveman added documentation documentation change can be for code and/or markdown pages Provider Specific to the Provider API labels May 27, 2020
@vperron
Copy link
Author

vperron commented May 27, 2020

I think you're right, the change is incorrect. Some colleagues and myself did indeed misread it.

I'll try and add some clarification to better reflect this point.

@schnuerle schnuerle added this to the 1.1.0 milestone Sep 22, 2020
@schnuerle schnuerle modified the milestones: 1.1.0, Future Oct 16, 2020
@schnuerle
Copy link
Member

@vperron is there a different change you'd like to propose here, or is it OK to close this PR in favor of a future PR aligned with the latest MDS release code?

@vperron
Copy link
Author

vperron commented Jan 26, 2021

As I stated in my latest comment this PR is actually invalid, I'll close it immediately.

@vperron vperron closed this Jan 26, 2021
@schnuerle schnuerle removed this from the Future milestone Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation change can be for code and/or markdown pages Provider Specific to the Provider API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants