-
Notifications
You must be signed in to change notification settings - Fork 293
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
Incorporate Best Practice Recommendations #271
Conversation
Adds best practices recommendations. Cleans up typos. Fixes broken anchors.
updates free_bike_status definition
Adds info on latency requirements, formatting changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions from proof reading.
Co-authored-by: Adrian Schoenig <adrian@schoenig.me>
restores vehicle_capacity, minor edits
fixes borken links and minor typos
gbfs.md
Outdated
@@ -267,7 +331,7 @@ Field Name | Required | Type | Defines | |||
|
|||
### vehicle_types.json *(added in v2.1-RC)* | |||
|
|||
The following fields are all attributes within the main "data" object for this feed. | |||
This file should be published by systems offering multiple vehicle types for rental, for example pedal bikes and ebikes. <br/>The following fields are all attributes within the main "data" object for this feed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be good to add stronger wording here to reflect the requirement of this file in these types of systems. Maybe something like:
This file must be published...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evansiroky I'm reluctant to make vehicle_types.json
a requirement when this is intended to be a non-breaking PR. While requiring it may not break down stream apps it seems like a significant change for producers. Currently it's conditionally required of systems who publish vehicle type data in free_bike_status.json
. I think we should revisit how it's defined in v3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused. In the Files section, it says that this file is conditionally required already. Seems like that same language ought to be used here to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the language from the Files section-
As it's currently defined under Files, systems publishing vehicle type information in free_bike_status
MUST publish vehicle_types.json
. If they are not including vehicle type information in free_bike_status
they would be exempted from that requirement regardless of the types of vehicles offered. This says that for those that operate multiple types of vehicles, publishing vehicle_types.json is something they SHOULD do. If that's not adequate, we could redefine vehicle_types.json
in v3.0 to require it of systems operating multiple types of vehicles
gbfs.md
Outdated
\- `num_docks_available` | Conditionally required <br/>*(as of v2.0)* | Non-negative integer | Required except for stations that have unlimited docking capacity (e.g. virtual stations) *(as of v2.0)*. Number of functional docks physically at the station. To know if the docks are accepting vehicle returns, see `is_returning`. | ||
\- `vehicle_docks_available` <br/>*(added in v2.1-RC)* | Conditionally Required | Array | This field is required in feeds where the [vehicle_types.json](#vehicle_typesjson) is defined and where certain docks are only able to accept certain vehicle types. If every dock at the station is able to accept any vehicle type, then this field is not required. This field's value is an array of objects. Each of these objects is used to model the number of docks available for certain vehicle types. The total number of docks from each of these objects should add up to match the value specified in the `num_docks_available` field. | ||
\- `station_id` | Yes | ID | Identifier of a station. See [station_information.json](#station_informationjson). | ||
\- `num_bikes_available` | Yes | Non-negative integer | The total number of vehicles of all types that are currently available and offered for rental at the station. If the `is_renting` boolean is set to`false` this number should be zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be conditionally required for valet stations, similar to num_docks_available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question, what I don't understand is if there were a valet station with an unlimited supply of vehicles, how would the operator indicate that there are any bikes available without using this field? In practice I don't think it matters that much because where i've seen valet service, the operator uses the station to initiate the rentals so it's constantly being refilled to maintain the value in num_bikes_available
minor edits, updates stations_status to align with changes in #261
fix typo
Change num_bikes_available and num_docks_available definitions to better align with v2.0
Updates vehicle_types.json definition to reflect changes in 2.1RC2
I hereby call a vote on this proposal. Voting will be open for 7 full calendar days until 11:59PM UTC on Monday, November 9th. |
Additional detail in description of vehicle_types.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to also update the system_pricing_plans documentation to clarify start/end interval points in pricing? When I looked at it recently it wasn't clear whether the start/end points in pricing are exclusive or inclusive, and exactly at which point the price increases.
E.g. the start documentation says "Number of kilometers that have to elapse before this segment starts applying." Does this mean that if start=1 and trip takes 1 min, we don't charge the user, but if the trip is taking 1.01 min we would charge the user? Similar confusion for the end field.
@@ -501,7 +564,7 @@ Example: | |||
``` | |||
|
|||
### system_hours.json | |||
Describes the system hours of operation. | |||
This optional file is used to describe hours and days of operation when vehicles are available for rental. If `system_hours.json` is not published it indicates that vehicles are available for rental 24 hours a day, 7 days a week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more clarification is needed that: if a system is outside of hours of operation, it should have no vehicles available in the realtime feeds (i.e. it should set num_bikes_available=0 and have no vehicles in free_bike_status.json). I'm not sure that is mentioned anywhere. The last sentence here makes it sound like consumers need to ingest system_hours just to determine if current real-time availability is outside of hours of operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if a system is outside of hours of operation, it should have no vehicles available in the realtime feeds (i.e. it should set num_bikes_available=0 and have no vehicles in free_bike_status.json).
The definition of num_bikes_available
was changed (discussion in #94, PR #195 )to say just the opposite of this. During off hours num_bikes_available
now remains constant so someone trying to plan a trip at 6:45 can see how many vehicles are going to be available at 7:00 when the system opens. Same should be true for free_bike_status
. In a station based system the operator could set is_renting
to false
in station_status
during the time when the system is closed but there's no equivalent in free_bike_status
so you would need to ingest system_hours
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to also update the system_pricing_plans documentation to clarify start/end interval points in pricing?
start
and end
definitions have been updated in pricing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I don't like that the realtime part of the spec does not reflect the realtime rental availability and consumers have to double-check with another mostly static feed, but I do respect conversation in #94. Would it make sense to add guidance to is_renting on whether this should be set to false outside of hours of operation? Currently the description seems to point towards it: "temporarily taken out of service".
Adds additional detail to pricing start and end fields
  \- `rate` <br/>*(added in v2.1-RC2)* | Yes | Float | Rate that is charged for each kilometer `interval` after the `start`. Can be a negative number, which indicates that the traveller will receive a discount. | ||
  \- `interval` <br/>*(added in v2.1-RC2)* | Yes | Non-Negative Integer | Interval in kilometers at which the `rate` of this segment is either reapplied indefinitely, or if defined, up until (but not including) `end` kilometer.<br /><br />An interval of 0 indicates the rate is only charged once. | ||
  \- `end` <br/>*(added in v2.1-RC2)* | Optional | Non-Negative Integer | The kilometer at which the rate will no longer apply.<br /><br /> If this field is empty, the price issued for this segment is charged until the trip ends, in addition to following segments. | ||
  \- `end` <br/>*(added in v2.1-RC2)* | Optional | Non-Negative Integer | The kilometer at which the rate will no longer apply *(exclusive)* e.g. if `end` is `20` the rate no longer applies at 20.00 km.<br /><br /> If this field is empty, the price issued for this segment is charged until the trip ends, in addition to following segments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this still doesn't make the rate function clear to me. It seems neither the start or the end minute/kilometer is included in the range. Assume per_min_pricing:
{start: 1, end: 3, rate: 1}, {start: 3, end: 4, rate: 0.5}
Is this how much trips would cost at these times:
- 1 min: 0
- 1 min 1 second to 2 min: 1
- 2 min: 1
- 2 min 1 second to 3 min: 2
- 3 min: 2
- 3 min 1 second: 2.5
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because start
is inclusive, your example {start: 1, end: 3, rate: 1}, {start: 3, end: 4, rate: 0.5} assuming interval
is 1 would result in:
0-59 sec: 0
1 min: 1
1 min 1 second to 1 min 59 sec: 1
2 min: 2
2 min 1 second to 2 min 59 sec: 2
3 min: 3
3 min 1 second to 3 min 59 sec: 3.5
4 min 3.5
To set per min pricing you would use { start: 0, interval: 1, rate: 1 } which would result in:
0-59 sec: 1
1 min to 1 min 59 sec: 2
2 min to 2 min 59 sec: 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add guidance to is_renting on whether this should be set to false outside of hours of operation?
added "Field should be set to false
during hours or days when the system is not offering vehicles for rent"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for start says: "kilometers (inclusive) that have to elapse before this segment starts applying". Note the "before" and "elapse" bits.
I.e. if start=1, this reads to me that 1 kilometer (inclusive) has to elapse before the segment starts applying. I.e. we have to get past the 1km mark. If it said exclusive, it would make more sense to me. But I would suggest chancing this sentence completely. How about: "The kilometer at which this segment rate starts being charged (inclusive)."
@@ -501,7 +564,7 @@ Example: | |||
``` | |||
|
|||
### system_hours.json | |||
Describes the system hours of operation. | |||
This optional file is used to describe hours and days of operation when vehicles are available for rental. If `system_hours.json` is not published it indicates that vehicles are available for rental 24 hours a day, 7 days a week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I don't like that the realtime part of the spec does not reflect the realtime rental availability and consumers have to double-check with another mostly static feed, but I do respect conversation in #94. Would it make sense to add guidance to is_renting on whether this should be set to false outside of hours of operation? Currently the description seems to point towards it: "temporarily taken out of service".
updates guidance on is_renting
fix typo
update to clarify pricing start definition
Thanks for documenting the best practices. Voting +1 from Google Maps |
+1 on behalf of Transit. |
+1 from SkedGo. |
+1 from IBI Group |
This vote has been extended until 11:59PM UTC on Thursday, November 12th. Please weigh in for or against the proposal, and include the organization for which you are voting in your comment. |
+1 from Lime |
+1 from Bird. |
+1 from PBSC |
This vote is now closed - and it passes Votes in favor: There we no votes against |
This proposal incorporates best practice recommendations into the specification. These recommendations have been developed over the past two years based on community feedback.
The objectives of incorporating GBFS Best Practices recommendations into the specification are:
These additions touch many areas of the specification but generally fall into the following three categories:
This proposal is intended to be non-breaking. There will be a second, smaller proposal following this one containing breaking changes.