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

Clarify the type of Localized URL #514

Merged
merged 4 commits into from
Dec 12, 2023
Merged

Conversation

tdelmas
Copy link
Contributor

@tdelmas tdelmas commented Jun 8, 2023

What is the issue and why is it an issue?

The type Localized String is used at a multiple places with an additional specification that text must be of the type URL. It is an issue because it makes the specification more verbose, so less readable.

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

I propose to define that Localized String where the text is a URL as Localized URL. The type in the header, so the type is more explicit.

But maybe the type Localized URL should be defined as A fully qualified URL to be even clearer?

image

@tdelmas
Copy link
Contributor Author

tdelmas commented Oct 25, 2023

Ping @richfab : Can I have your opinion on this change? Do you think it falls in the "Editorial changes" category?

@richfab
Copy link
Contributor

richfab commented Oct 25, 2023

Hi @tdelmas, sorry for the delay.
Could you please give more context as to what is the issue and why it's an issue please?
Are you suggesting to replace the type URL by Localized URL in the spec?
Thank you in advance!

@tdelmas
Copy link
Contributor Author

tdelmas commented Dec 7, 2023

(sorry for the late reply too!)

  • What is the issue: The type Localized String is used at a multiple place with an additional specification that text must be of the type URL. It is an issue because it makes the specification more verbose, so less readable.

I propose to define that "Localized String where the text is an URL" as "Localized URL" the type in the header, so the type is more explicit.

But maybe the type "Localized URL" should be defined as "A fully qualified URL" to be even clearer?

@richfab
Copy link
Contributor

richfab commented Dec 8, 2023

Tagging recent contributors to get your opinion: @mplsmitch, @testower, @cmonagle, @simonsolnes, @ezmckinn, @AntoineAugusti, @ArashMansouri, @jkurzanski.
Thank you!

gbfs.md Outdated Show resolved Hide resolved
gbfs.md Outdated Show resolved Hide resolved
@testower
Copy link
Contributor

testower commented Dec 8, 2023

I agree it is confusing. And made further confusing by the fact that it is not actually the URL itself that is localized (or translated), but the URL points the a version of the content that is localized according to the language code...

@AntoineAugusti
Copy link
Contributor

👍 transport.data.gouv.fr, a simple and useful clarification.

@jkurzanski
Copy link

👍 for Vulog

tdelmas and others added 3 commits December 8, 2023 09:38
Co-authored-by: Antoine Augusti <[email protected]>
Co-authored-by: Antoine Augusti <[email protected]>
@testower
Copy link
Contributor

testower commented Dec 8, 2023

👍 from Entur as well

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.

One minor typo to fix. LGTM otherwise!

Thank you @tdelmas for this helpful contribution 💯

gbfs.md Outdated Show resolved Hide resolved
Co-authored-by: Fabien Richard-Allouard <[email protected]>
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!
Merging now 🚀

@richfab richfab merged commit 3208248 into MobilityData:master Dec 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants