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

Change field vehicle_type_id to vehicle_type_ids on geofencing_zones.features - version v3.0-RC #529

Closed
1 of 3 tasks
pgiki opened this issue Aug 15, 2023 · 5 comments · Fixed by #542
Closed
1 of 3 tasks

Comments

@pgiki
Copy link

pgiki commented Aug 15, 2023

I'm new to GBFS but thrilled about discovering this incredible shared mobility specification. As a huge fan of shared micro-mobility, I'm currently developing an open-source Django-based REST API using the GBFS schema. I'm finalizing the database schema and aiming to release the project soon.

I have a query regarding the "vehicle_type_id" within geofencing_zones.features (v3.0-RC). It seems to represent multiple vehicle type IDs, but elsewhere, it's referred to as "vehicle_type_ids". I'm curious why the singular form is used here; changing the key to vehicle_type_ids would likely sound more appropriate.

Please describe some potential solutions you have considered (even if they aren’t related to GBFS).

Change field vehicle_type_id to vehicle_type_ids on geofencing_zones.features

Is your potential solution a breaking change?

  • Yes
  • No
  • Unsure
  {
    "geometry": { "#": "... Polygon A ..." },
    "properties": {
      "rules": [
        {
          "vehicle_type_id": ["bike", "scooter"], //change this field to "vehicle_type_ids"
          "ride_through_allowed": true
        }
      ]
    }
  },
  {
    "geometry": { "#": "... Polygon B ..." },
    "properties": {
      "rules": [
        {
          "vehicle_type_id": ["scooter"],
          "ride_through_allowed": false
        }
      ]
    }
  }
],
"global_rules": [
  {
    "ride_through_allowed": false
  }
]```
@josee-sabourin
Copy link
Contributor

Hi @pgiki, welcome!

Great catch, this might have slipped through the cracks when we were updating geofencing_zones in v3.0, and should be plural to be consistent with the rest of the specification. Because this changes the name of a field, this would be considered a breaking change. Given that v3.0-RC has not been made official, we can roll this change into v3.0-RC2 along with #497 and #522.

To get this change into v3.0-RC2, it would need to pass a vote, to do so you can open a Pull Request. Governance states that the Pull Request must be open for a minimum of 7 days, at which point you, as the Advocate, can call a vote. Alternatively, MobilityData, as the maintainers, can take on the role of the Advocate and open the Pull Request and call the vote.

@pgiki
Copy link
Author

pgiki commented Aug 16, 2023

Hi @josee-sabourin, thanks for the quick response and for seeing the suggestion useful. Alternative 2 where MobilityData opens the pull request is fine with me.

@mplsmitch
Copy link
Collaborator

This is something that's repeatedly caused confusion since vehicle types were introduced in v2.1. Having fields ( vehicle_type_id & vehicle_type_ids) with such similar names is a bad idea. I'd be supportive of renaming all the vehicle_type_ids to something else. For example we could change them to types_of_vehicles, the field definitions would remain the same. What do other people think, does this bother you as much as it does me? If folks agree I'll write the PR.

@pgiki
Copy link
Author

pgiki commented Aug 17, 2023

For naming consistency I would say vehicle_type_ids is fine for this purpose. This is because other fields use the same singular-plural format. For example pricing_plan_id vs pricing_plan_ids on vehicle_types.json.

@josee-sabourin
Copy link
Contributor

Closing this, please continue the discussion in #542

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants