-
Notifications
You must be signed in to change notification settings - Fork 290
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
Forbid empty Strings #636
Forbid empty Strings #636
Conversation
Before submitting to a vote, do you think that change could be in 3.2 ? It is technically a breaking change, but I think it worth it to improve the GBFSv3. |
Hi @tdelmas, I think we should avoid making an exception to the change process. If the community wants to make the change in v3.2-RC, we could consider:
|
Signed-off-by: GitHub <[email protected]>
@richfab Do you think the update I pushed is compatible with the next v3.X-RC? Do you have an opinion about it? |
I'm more inclined to support this as a requirement (MUST) in the next major version. I'd like to hear your reasoning for this interim step. I think using optional SHOULD in place of MUST to get it in a point release sooner is unlikely to result in changes to existing data sets. We have lots of optional fields with SHOULD in their definitions and few of them show up in feeds. I also think this would be a whole lot cleaner if you only changed the |
@mplsmitch thank you for the feedback.
That's why I think we should push the "SHOULD" as soon as possible. But if the community wants to release a major version soon, maybe that step is not necessary?
That's what I did with the last commit (I only changed
I don't think it is necessary at the Localized String definition references the String one. |
Release timing is limited by the Governance which states: So I think the earliest this could happen is 1 yr from the release of v3.0 meaning spring of 2025. A minor release could happen in fall of 2024 but the next step is getting through a vote. Releases typically contain a number of changes. We have never done a release based on a single change. Right now there are no PRs that have been put to a vote for the next release. |
I think that waiting for the next MAJOR release (with "MUST not be empty") is reasonable for this change. On the other hand, @tdelmas you are welcome to open a vote if you think it is useful to try to pass this change in a MINOR version (with "SHOULD not be empty"). Thank you @tdelmas and @mplsmitch for your involvement in this issue 🙏 |
I hereby call a vote on this proposal. Voting will be open for 10 full calendar days until 11:59PM UTC on Sunday, September 22, 2024. |
+1 from Where To? / FutureTap. |
Assuming we are now voting for the MUST-variant of the proposal: +1 from Entur |
+1 from Transit |
+1 OpenTripPlanner In practise this is already implemented, since we reject entities that have empty names for example. Although I'm not sure if we are voting for the MUST on the next major version or a SHOULD. |
@leonardehrenfried This is a vote for "Specify that strings (including IDs) MUST NOT be empty." (breaking change, so for the next major) |
🗳️ Voting on this Pull Request closes in 2 calendar days (11:59PM UTC on September 22). |
This vote has now closed, and it passes! Votes in favor:
There were no votes against. This change will be part of v4.0-RC, planned to be released when there will be enough breaking changes to constitute a new MAJOR version (likely in Nov 2025), as per the version release cycle in the governance. Thank you for your involvement in the GBFS spec 🙏 |
Fix #600
See also https://mobilitydata-io.slack.com/archives/CNXA9ASBV/p1715598631556219
What is the proposal?
Specify that strings (including IDs) MUST NOT be empty.
Is this a breaking change?
Which files are affected by this change?
All of them (because of the IDs)
Examples of affected feeds
https://mobilitydata-io.slack.com/archives/CNXA9ASBV/p1708449123435399?thread_ts=1708429919.754579&cid=CNXA9ASBV