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

Forbid empty Strings #636

Closed
wants to merge 3 commits into from
Closed

Forbid empty Strings #636

wants to merge 3 commits into from

Conversation

tdelmas
Copy link
Contributor

@tdelmas tdelmas commented May 16, 2024

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?

  • Yes
  • No
  • Unsure

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

Examples for feeds currently using (very few) empty station names:

@tdelmas
Copy link
Contributor Author

tdelmas commented May 30, 2024

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.
Thoughts?

@richfab
Copy link
Contributor

richfab commented May 30, 2024

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:

  1. Making the change non-breaking with "SHOULD not be empty" instead of "MUST not be empty".
  2. Asking producers to fill in the missing values.

@tdelmas
Copy link
Contributor Author

tdelmas commented Aug 20, 2024

@richfab Do you think the update I pushed is compatible with the next v3.X-RC? Do you have an opinion about it?

@mplsmitch
Copy link
Collaborator

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 String field type definition (and possibly Localized String) and left all the other fields definitions that contain strings as they are.

@tdelmas
Copy link
Contributor Author

tdelmas commented Aug 20, 2024

@mplsmitch thank you for the feedback.

I'd like to hear your reasoning for this interim step.

  • "MUST" has its place in the next major version
  • In the interim, it's important to;
    • Avoid more feeds with empty strings
    • Encourage current feeds with empty strings to remove them
    • Or discover corner-cases that we missed where empty strings are indeed useful

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?

I also think this would be a whole lot cleaner if you only changed the String field type definition

That's what I did with the last commit (I only changed string into String for IDs to emphasize they refer to that definition).

(and possibly Localized String)

I don't think it is necessary at the Localized String definition references the String one.

@mplsmitch
Copy link
Collaborator

But if the community wants to release a major version soon, maybe that step is not necessary?

Release timing is limited by the Governance which states:
"MAJOR releases are limited to no more than once a 12 month period. "

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.

@richfab
Copy link
Contributor

richfab commented Aug 27, 2024

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 🙏

@tdelmas
Copy link
Contributor Author

tdelmas commented Sep 12, 2024

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.
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.

@futuretap
Copy link
Contributor

+1 from Where To? / FutureTap.

@testower
Copy link
Contributor

Assuming we are now voting for the MUST-variant of the proposal: +1 from Entur

@cmonagle
Copy link
Contributor

+1 from Transit

@leonardehrenfried
Copy link
Contributor

leonardehrenfried commented Sep 13, 2024

+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.

@tdelmas
Copy link
Contributor Author

tdelmas commented Sep 13, 2024

@leonardehrenfried This is a vote for "Specify that strings (including IDs) MUST NOT be empty." (breaking change, so for the next major)

@richfab
Copy link
Contributor

richfab commented Sep 20, 2024

🗳️ Voting on this Pull Request closes in 2 calendar days (11:59PM UTC on September 22).
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.
Thank you 🙏

@richfab
Copy link
Contributor

richfab commented Sep 23, 2024

This vote has now closed, and it passes!

Votes in favor:

  • Where To? / FutureTap (consumer)
  • Entur (producer)
  • Transit (consumer)
  • OpenTripPlanner (consumer)

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 🙏

@richfab richfab closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required strings
8 participants