-
Notifications
You must be signed in to change notification settings - Fork 1
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 provider free2move / free2move_stuttgart #194
Conversation
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'd like to tweak the geofencing_zones.json
docs and the caching implementation. Otherwise just small comments.
# We build dicts with VINs as key to merge on them | ||
cached_vehicles_map = {v['vin']: v for v in cached_response.get('vehicles', {})} | ||
delta_vehicles_map = {v['vin']: v for v in delta_response.get('vehicles', {})} | ||
merged_vehicles_map = cached_vehicles_map | delta_vehicles_map |
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.
Does delta_response
ever contain information that a vehicle is not available (as in "is not rentable", not as in "is currently rented") anymore? If it does, how would we take that information into account when merging?
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 API publishes freeForRental
, so unavailable vehicles should be published in the delta. Using only delta feeds nvertheless could result in "ghost cars", which are not published any longer. Performing a full update every hour should limit this at least.
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.
Using only delta feeds nvertheless could result in "ghost cars", which are not published any longer. Performing a full update every hour should limit this at least.
I recommend adding this information to the docs.
headers: Optional[dict[str, str]] = None, | ||
json: Optional[Dict] = None, | ||
timeout: int = 5, | ||
user_agent: str = 'x2gbfs +https://github.com/mobidata-bw/', |
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.
Shall we make this configurable via an environment variable?
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.
Yes, this should become a proper PR
Co-authored-by: Jannis R <[email protected]>
Co-authored-by: Jannis R <[email protected]>
Co-authored-by: Jannis R <[email protected]>
Co-authored-by: Jannis R <[email protected]>
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.
This converter works for all locations, right? I need to clarify, if we can provide other locations than stuttgart.
`vehicle_image` | `row['imageUrl']`, `{density}` is set to `2x`, as only one image may be specified. | ||
`make` | extracted from `row['buildSeries']` and mapped in the code. | ||
`model` | extracted from `row['buildSeries']` and mapped in the code. | ||
`color` | `row['primaryColor']` mapped to `weiß`, `silber`, `schwarz` or `grau`. In case no mapping is defined, we return the last part of the `imageUrl`, as this seems to contain the color string (in English). If no `imageUrl` is provided, we return `unbekannt`. |
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.
Do we need to introduce a value 'unbekannt'? I suggest to omit the color attribute if no color information is available.
`vehicle_type_id` | see section vehicle_types | ||
`last_reported` | curent time | ||
`current_range_meters` | `row['remainingRange']`, if set | ||
`current_fuel_percent` | `row['fuelLevel'] / 100.0`, or `.25` if unset |
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.
do we need to define a default value? current_fuel_percent is optional.
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.
According to documentation, fuelLevel
is not optional and should provided for every vehicle. I remove the default handling.
It should. But note that other areas may use different |
…/free2move # Conflicts: # CHANGELOG.md # x2gbfs/x2gbfs.py
This PR adds
Free2moveProvider
.NOTE: the generated feed publishes free floating vehicles using the original vehicle identifiers (
VIN
) asbike_id
s and inrental_uri
s. In consequence, it does not comply to the GBFS spec, which requires thatThis feed must not be published openly without handling of this issue.