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

Add stop references to trips and status_changes #442

Merged

Conversation

avatarneil
Copy link
Contributor

Explain pull request

Resolves #428 following @jfh01's suggestion to use GeoJSON Foreign Members. Relates pretty closely to the open PR #427, and is dependent on that being merged first.

Is this a breaking change

  • No, not breaking

Impacted Spec

  • provider

@avatarneil avatarneil requested a review from a team as a code owner February 5, 2020 18:42
@avatarneil avatarneil requested a review from a team February 5, 2020 18:42
@thekaveman
Copy link
Collaborator

Two comments:

  1. I know @jfh01 suggested the use of Foreign Members, and I don't see a specific issue there. I just wonder if there is any reason not to extend our existing use of the properties object already present in GeoJSON Features?

  2. Regardless of the answer to 1, the schema will need to be updated to handle the additional field.

@sarob sarob added enhancement New feature or request Provider Specific to the Provider API labels Feb 6, 2020
@sarob sarob added this to the Future milestone Feb 6, 2020
@ascherkus
Copy link

Chiming in ... I'm also a bit hesitant on relying on GeoJSON foreign members vs. properties (similar to the hesitancy mentioned in #441).

Given:

... I'm inclined to suggest sticking with properties for Feature-specific metadata, unless @jfh01 has some additional insight/experience into foreign members :)

Alternatively start/end stop ids could be expressed higher in the hierarchy alongside start/end_time -- the tradeoff there would being unable to express a trip that stopped at multiple stops (unclear to me if such a trip would exist, or if that would be represented as two trips with separate start/stop pairs). If I had to pick, I think sticking with Feature properties with schema validation accomplishes the same goal while retaining flexibility vs. adding more named fields at the top-level trips object.

@jfh01
Copy link
Contributor

jfh01 commented Feb 7, 2020

No particular insight here. I'm happy with either direction. Thanks!

@avatarneil avatarneil force-pushed the trips-route-stop-implementation branch from 7506e12 to baf829b Compare February 13, 2020 17:59
@avatarneil
Copy link
Contributor Author

avatarneil commented Feb 13, 2020

Updated the PR to move the stop references to the top level of thetrips and status_changes objects.

@avatarneil avatarneil changed the title Add GeoJSON Foreign Member stop_id to trips Add stop references to trips and status_changes Feb 13, 2020
@thekaveman
Copy link
Collaborator

@rgangopadhya

Hmm, I do think this could be useful, though. I must've missed it on the call but did we agree to remove this?

I think we decided to move it nearer to the geometry, inside the properties object of relevant GeoJSON Feature (status_change.event_location, the first/last point in trip.route). This PR has been updated to reflect the changes for trip.route but it looks like we still need something for status_changes.event_location @avatarneil

@avatarneil
Copy link
Contributor Author

Hi @thekaveman, I've updated the event_location definition. Sorry for the delay!

@thekaveman
Copy link
Collaborator

I rebased on the latest dev to incorporate Reconciliation changes. Let's wait to merge this after #427 so we can update references to Stops in the Provider README.

@thekaveman thekaveman force-pushed the trips-route-stop-implementation branch from daa70cc to 0545510 Compare June 30, 2020 00:07
@avatarneil avatarneil requested a review from a team June 30, 2020 00:07
Copy link
Member

@schnuerle schnuerle left a comment

Choose a reason for hiding this comment

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

Thanks for these updates everyone.

@schnuerle schnuerle merged commit 79b5870 into openmobilityfoundation:dev Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Provider Specific to the Provider API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify /trips to represent docked start/end locations
6 participants