-
Notifications
You must be signed in to change notification settings - Fork 177
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
Clarify stops.zone_id conditional requirement for route-based fare_rules #432
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Hi @westontrillium , Thanks for working on a proposal and opening this PR. I’m bringing over @bdferris-v2's comment made on Issue #429, considering this point, perhaps it would good to could consider the option of making this field a recommended field instead of required? It could be recommended if zone-based info is provided (i.e. keeping the text you are proposing but replacing In any case, I think in both options solve the problem of having stops without an assigned Any thoughts? |
Good points from @bdferris-v2. I also can't think of a scenario where this field would be strictly required, especially universally. In light of this, I think I'll wait to give others a chance to share their opinion, but I'm happy to make the eventual change to the PR to make this |
Updating with most recent commits
I went ahead and updated the PR to change the conditional requirement to optional. At this point, I believe a vote may be called @Sergiodero? |
That's right @westontrillium! we need to cover a minimum 7-day period for discussion before calling a vote as per the Specification amendment process. Since this change is not bringing any new functionality or changing an existing one, I think it's okay to open a vote without testing (but happy to hear if someone else has a different opinion). |
I am opening a vote to merge this Pull Request. Voting ends on 2024-04-30 at 23:59:59 UTC. |
+1 Google |
+1 from Spare Labs |
❓ Isn't |
@e-lo Not universally. The zone that |
I guess we need to differentiate between a field being optional (existing as a column) vs a field being nullable (have blank values). If you have any |
I don't think so. If you have any I don't know how the presence of the column—empty or otherwise—is relevant. Dependencies are always based on the values within the fields. |
I don't dispute that, but the term conditionally required has been used throughout the spec to note this in many places and I'd just want to make sure we are being consistent. |
Hmm... I just went through all instances of "conditionally required" in the spec, and I didn't find a single case where "conditionally required" is invoked for the presence of a primary key if a foreign id field references it. |
The @mbta for a time published an internal-only version of the v1 fare_rules.txt for our OpenTripPlanner instance. It appears we were leaving +1 from @mbta |
+1 from Ito |
+1 from Ualabee |
@westontrillium It looks like this PR has passed and is now adopted to GTFS. Congratulations on passing your first PR. +1 Google (@bdferris-v2) We have 5 votes for the change, 0 votes against the change, and 0 votes in abstention. |
Related issue: #429
This pull request modifies the conditional requirement for
stops.zone_id
to allow for the scenario in which the stop is only assigned totrip_id
s whoseroute_id
is present in a fare_rules.txt record with onlyfare_id
androute_id
defined (route-based fares).