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

Specify IDs characters range restrictions #545

Merged
merged 3 commits into from Nov 2, 2023

Conversation

tdelmas
Copy link
Contributor

@tdelmas tdelmas commented Sep 20, 2023

This PR clarify and restrict the characters allowed in IDs. See #541 for additional details.

Those edits have multiple goals:

  • Clarify the current specification that is too vague
  • Ensure interoperability, it MUST be possible to store and compare IDs easily and without pitfall
  • Ensure that IDs are easy to manipulate by humans, because even if the format is design for machines, IDs are often used by humans to debug, so they SHOULD be easy to read, write, copy and compare, regardless of the keyboard layout or system used.

With that specification, all IDs present in system.cvs should stay compliant (excepts those using spaces 0x20, who are already not compliant with the current specification).

Also, all others shared mobility systems that I have seen (GBFS or not) are compatible with both restrictions.

Does anyone think that IDs should be restricted further, such as A-Za-z0-9_-:? (And consider that existing system that are using .@/ are compliant because "SHOULD" means "that there may exist valid reasons in particular circumstances to ignore a particular item", and at least "legacy" may fit into that.

Fix #541

Is this a breaking change?

  • Yes

Which files are affected by this change?

Most of them

This PR clarify and restrict the characters allowed in IDs. See MobilityData#541 for additional details.

Those edits have multiple goals:
- Clarify the current specification that is too vague
- Ensure interoperability, it MUST be possible to store and compare IDs simply
- Ensure that IDs are easy to manipulate by humans, because even if the format is design for machines, IDs are often used by humans to debug, so they SHOULD be easy to read and write, regardless of the keyboard layout or system used.

With that specification, all IDs present in `system.cvs` should stay compliant (excepts those using spaces `0x20`, who are already not compliant with the current specification).

Also, all other shared mobility systems that I have seen (GBFS or not) are compatible with both restrictions (again, except for spaces in a case that looks like an error).

Does anyone think that IDs should be restricted further, such as `A-Za-z0-9_-:`? (And consider that existing system that are using `.@/` are compliant because "SHOULD" means "that there may exist valid reasons in particular circumstances to ignore a particular item", and at least "legacy" may fit into that.   

Fix MobilityData#541
@josee-sabourin josee-sabourin mentioned this pull request Sep 25, 2023
1 task
@josee-sabourin
Copy link
Contributor

josee-sabourin commented Sep 25, 2023

tagging others for visibility: @cmonagle @viestat @ArashMansouri @futuretap @AntoineAugusti @ezmckinn @fbouchPBSC @Carl-NM @bdferris-v2 @kulovan

@futuretap
Copy link
Contributor

+1 looks good

@AntoineAugusti
Copy link
Contributor

+1 as well

@richfab richfab added proposal:breaking gbfs.md v3.0-RC2 Candidate change for GBFS 3.0 (Major release) - 2nd pass labels Sep 26, 2023
@PierrickP
Copy link

(+1) Tom is my colleague, and we work on it together

@cmonagle
Copy link
Contributor

+1 From Transit

@testower
Copy link
Contributor

+1 from Entur

gbfs.md Outdated
@@ -202,7 +202,8 @@ Example: The `rental_methods` field contains values `creditcard`, `paypass`, etc
* ID - Should be represented as a string that identifies that particular entity. An ID:
* MUST be unique within like fields (for example, `station_id` MUST be unique among stations)
* Does not have to be globally unique, unless otherwise specified
* MUST NOT contain spaces
* MUST be in the ASCII printable caracter range, space excluded (0x21 to 0x7E) https://en.wikipedia.org/wiki/ASCII#Printable_characters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

caracter => character

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

@Cj-Malone
Copy link
Contributor

+1

caracter => character (thanks @futuretap)
@tdelmas
Copy link
Contributor Author

tdelmas commented Oct 2, 2023

(to conform with https://github.com/MobilityData/gbfs/blob/master/governance.md)

I hereby call a vote on this proposal. Voting will be open for 10 full calendar days until 11:59PM UTC on Oct 12.
Please vote for or against the proposal, and include the organization for which you are voting in your comment.
Please note if you can commit to implementing the proposal.

@tdelmas
Copy link
Contributor Author

tdelmas commented Oct 5, 2023

@futuretap @AntoineAugusti @cmonagle @testower @Cj-Malone Thank you for the +1. I'm told that to as official as possible, votes should be done after the call to vote.

I'll be grateful if you could confirm your vote before 11:59PM UTC on Oct 12 and include the organization for which you are voting in your comment.

@AntoineAugusti
Copy link
Contributor

+1, transport.data.gouv.fr

@futuretap
Copy link
Contributor

+1 from Where To? / FutureTap

@ArashMansouri
Copy link
Contributor

+1 from Lyft

@josee-sabourin
Copy link
Contributor

Voting on this PR closes in 2 calendar days. Please vote for or against the proposal, and include the organization for which you are voting in your comment. Please note if you can commit to implementing the proposal.

@testower
Copy link
Contributor

+1 from Entur

@bdferris-v2
Copy link
Contributor

+1 from Google

@josee-sabourin
Copy link
Contributor

josee-sabourin commented Oct 13, 2023

This vote has now closed, and it passes!

Votes in favor:
Transport.data.gouv (consumer)
Whereto.app (consumer)
Lyft (producer)
Entur (consumer)
Google (consumer)

There were no votes against.

This change will be added to v3.0-RC2, which will be released in the coming weeks.

Copy link
Contributor

@richfab richfab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

gbfs.md Outdated Show resolved Hide resolved
@richfab richfab merged commit d0acc51 into MobilityData:master Nov 2, 2023
1 check passed
@richfab richfab changed the title Specify IDs caracters range restrinctions Specify IDs caracters range restrictions Nov 2, 2023
@richfab richfab changed the title Specify IDs caracters range restrictions Specify IDs characters range restrictions Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gbfs.md proposal:breaking v3.0-RC2 Candidate change for GBFS 3.0 (Major release) - 2nd pass Vote Passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GBFS ID type definition