Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API #6272
base: dev-2.x
Are you sure you want to change the base?
Add currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API #6272
Changes from 32 commits
7fb3810
b61c9f3
1fa55b0
4686f7c
e93c828
49c4682
54292e2
851c8b5
d26799f
e0bd36a
e9a572d
498d547
eda75c8
3e7bf5b
7d15f00
9b9b585
c90bdd4
346449b
6edf98b
0b1920d
54b0bc0
f3e0a71
26d8d7d
4788055
ff8411a
6778169
62e671f
f7d6b36
4312caf
cd38992
17a9d79
c941f62
742bcd0
c5a998d
9e30029
c00e615
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Since we are always rounding this to int, should we just do the rounding when reading in the value and store it as int in this class since it requires less space? Do we have a need for less than meter precision from this class later?
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.
You're right, we don't need more precision than int. The reason I chose to use a Double was that the GBFS standard uses a float for it.
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.
@t2gran suggested we should store the distance as millimeter integer. You can then convert it to int meters in this method.
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.
In this case aren't more suited centimeters or decimeters? I don't think we need a millimeter of precision.
With centimeter we could represent more kilometers (21.000 against 2.100) with integer and we would still have enough precision.
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 added this check, can you double check if it's right?
I'm not sure but
vehicle_type_id
is REQUIRED if the vehicle_types.json file is defined, that file is REQUIRED for systems withfree_bike_status.json
and if this file is not included then all vehicles are non motorized bicycles.Therefore if the vehicleTypeId is not present in the vehicleTypes map I can assume that the file
vehicle_types.json
is not present and all vehicles are not motorized, so the propulsion type is human and range is not needed.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 this is correct, but this is in an area where a lot of feeds get things wrong so this validation might cause issues but I'm not sure if I'm against this or not.
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 spoke to @hbruch about this and he said that there are a number of feeds that don't include it where they should but he is in favour of enforcing the spec anyway. If we are not strict with data producers, they will never learn.