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

Use RFC3339 for Timestamp #522

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Conversation

leonardehrenfried
Copy link
Contributor

@leonardehrenfried leonardehrenfried commented Jul 17, 2023

If you are new to the specification, please introduce yourself (name and organization/link to GBFS). It’s helpful to know who we're chatting with!

I'm a core developer of OpenTripPlanner, a multimodal transit routing engine with GBFS support.

What problem does your proposal solve? Please begin with the relevant issue number. If there is no existing issue, please also describe alternative solutions you have considered.

Right now GBFS uses seconds since the epoch as a timestamp, for example in the last_updated field. While this practice is pretty wide-spread, it makes debugging feeds a little harder than necessary.

Every time I need to check the last_updated field I need to copy the epoch seconds, google "epoch to time converter", paste the seconds to get a usable date time.

While this isn't an impossible task, I think there are alternatives that are both human-readable and unique so that machines can identify a universal point in time.

What is the proposal?

Replacing POSIX time with RFC3339 makes a much more pleasant debugging experience for humans and doesn't add ambiguity for computers.

An example of RFC3339 time is 2023-07-17T13:45:29+02:00.

Is this a breaking change?

  • Yes
  • No
  • Unsure

Which files are affected by this change?

All of them. 😳

Timezone

It was suggested in #519 that we should limit the offset/timezone to UTC, so an offset of 0 or a Z at the end.

Personally, I'm not in favour of this restriction as you should use a proper date parsing library which handles all offsets, but as time is tight I would like to keep the discussion to a minimum and not let this point block the proposal.

EDIT: Consensus was reached to include the offset/timezone in this PR.

@josee-sabourin
Copy link
Contributor

Tagging those who were active in #519
@mplsmitch @sven4all @benwedge @testower @futuretap

@benwedge
Copy link

Timezone
It was suggested in #519 that we should limit the offset/timezone to UTC, so an offset of 0 or a Z at the end.

Personally, I'm not in favour of this restriction as you should use a proper date parsing library which handles all offsets, but as time is tight I would like to keep the discussion to a minimum and not let this point block the proposal.

IMO we should just get it right the first time. If it's going to be a breaking change and every language has a library that handles this nicely why not just do it?

@leonardehrenfried
Copy link
Contributor Author

IMO we should just get it right the first time. If it's going to be a breaking change and every language has a library that handles this nicely why not just do it?

That would of course be okay for me.

@testower
Copy link
Contributor

I agree with @benwedge let us not do it half way.

@futuretap
Copy link
Contributor

I support the proposal, including variable offsets.

@leonardehrenfried
Copy link
Contributor Author

Thanks all for chiming in. I will wait until the end of today and if no one shows up voicing opposition to variable offsets I will update the PR.

@leonardehrenfried
Copy link
Contributor Author

Since no-one has shown up who is against variable offsets, I have updated the PR.

gbfs.md Outdated Show resolved Hide resolved
gbfs.md Outdated Show resolved Hide resolved
@josee-sabourin
Copy link
Contributor

I (as proxy for @leonardehrenfried who is on vacation) hereby call a vote on this proposal. Voting will be open for 10 full calendar days until 11:59PM UTC on August 5th.
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

testower commented Aug 1, 2023

Entur votes in favor of this proposal and will implement it

@futuretap
Copy link
Contributor

We (Where To?) are in favor, too, and will implement it.

@AntoineAugusti
Copy link
Contributor

transport.data.gouv.fr is in favour and will implement it (for feeds we consume)

@benwedge
Copy link

benwedge commented Aug 3, 2023

Joyride (provider) is in favour and will implement this when we move to 3.x

@leonardehrenfried
Copy link
Contributor Author

Not sure if my vote counts but OpenTripPlanner will implement this via Entur's Java library.

@josee-sabourin
Copy link
Contributor

This vote has now closed, and it passes!

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

There were no votes against.

@richfab
Copy link
Contributor

richfab commented Aug 21, 2023

Datetime is currently ISO 8601 notation in the spec. Should Datetime be replaced by Timestamp (RFC3339)?

Datetime definition in the spec:
https://github.com/MobilityData/gbfs/blob/master/gbfs.md?plain=1#L195

Datetime only usage is in vehicle_status.json for available_until field:
https://github.com/MobilityData/gbfs/blob/master/gbfs.md?plain=1#L956

@testower
Copy link
Contributor

Good catch @richfab - I think for most practical purposes they are the same, but the spec should definitely be consistent. The ISO is very broad, so I would be in favor of restricting it to the RFC (which is a profile of the ISO standard?)

@leonardehrenfried
Copy link
Contributor Author

leonardehrenfried commented Aug 23, 2023

My suggestion is to combine the two, so all of them will be of type Timestamp or DateTime.

I have a slight personal preference for the name DateTime but I'm fine with both.

@leonardehrenfried
Copy link
Contributor Author

@josee-sabourin What is the process of merging this PR?

I would prefer to merge this and then make a follow up where I combine the two types since that would be a non-breaking change.

@josee-sabourin
Copy link
Contributor

@leonardehrenfried currently we are waiting to merge pending a few more potential breaking changes to limit the number of v3.0 release candidates we have.

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

hbruch commented Sep 14, 2023

Note that RFC3339 allows T and Z also to be written in lowercase, T might also be left out. Should the spec prescribe uppercase T and Z or allow these variations? In the latter case, the regex in gbfs-json-schema of course should reflect this.

@richfab
Copy link
Contributor

richfab commented Sep 26, 2023

Hello @hbruch,

Great question. For reference, this is the extract from the RFC3339 spec:

NOTE: Per [ABNF] and ISO8601, the "T" and "Z" characters in this syntax may alternatively be lower case "t" or "z" respectively.
This date/time format may be used in some environments or contexts that distinguish between the upper- and lower-case letters 'A'-'Z' and 'a'-'z' (e.g. XML). Specifications that use this format in such environments MAY further limit the date/time syntax so that the letters 'T' and 'Z' used in the date/time syntax must always be upper case. Applications that generate this format SHOULD use upper case letters.
NOTE: ISO 8601 defines date and time separated by "T". Applications using this syntax may choose, for the sake of readability, to specify a full-date and full-time separated by (say) a space character.

What is everyone's opinion about:

  1. Restricting T and Z to upper case or not?
  2. Allowing T to be left out or not?

Thank you!

@leonardehrenfried
Copy link
Contributor Author

leonardehrenfried commented Sep 26, 2023

I would be fine with not supporting the full flexibility of the spec and require them both to be mandatory and upper case. (Z can of course be replaced with an offset).

@futuretap
Copy link
Contributor

We're strongly preferring to restrict T and Z to upper case and require it not to be left out. Technical reason: Swift Decodable requires this strict format.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gbfs.md 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.

9 participants