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

Add minimal value to all timestamp json definition #648

Merged

Conversation

tonial
Copy link
Contributor

@tonial tonial commented May 27, 2021

The goal is to prevent the easy to make mistake of using a second
timestamp instead of a millisecond timestamp.

This PR implements issue #645

@tonial tonial requested a review from a team May 27, 2021 11:58
@tonial tonial requested a review from a team as a code owner May 27, 2021 11:58
@tonial tonial requested a review from a team May 27, 2021 11:58
The goal is to prevent the easy to make mistake of using a second
timestamp instead of a millisecond timestamp.
@tonial tonial changed the base branch from main to dev May 27, 2021 12:01
@schnuerle schnuerle added Agency Specific to the Agency API Schema Implications for JSON Schema or OpenAPI Provider Specific to the Provider API labels May 27, 2021
@schnuerle
Copy link
Member

Hi @thekaveman could you take a look at this? Is it ok to require all of these values exist and be greater than 0? Thanks.

@schnuerle schnuerle requested a review from thekaveman May 27, 2021 15:11
@schnuerle schnuerle added this to the 1.2.0 milestone May 27, 2021
@thekaveman thekaveman added the Policy Specific to the Policy API label May 28, 2021
Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Important to note this change doesn't impact any requirement for timestamps in any specific endpoint. It only changes the definition of a timestamp to be always greater than or equal to Jan 1, 2018 00:00:00 UTC.

A good example is in Provider /trips, the optional publication_time field. This change does not make the field required, it just dictates the lower bound if a value is given.

The change could impact testing scenarios or synthetic data generation that isn't using realistic time values, but IMHO is unlikely to impact real-world data producers in any meaningful way.

Copy link
Member

@schnuerle schnuerle left a comment

Choose a reason for hiding this comment

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

Thanks so much for this, looks good!

@schnuerle schnuerle merged commit 37e5e1f into openmobilityfoundation:dev Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agency Specific to the Agency API Policy Specific to the Policy API Provider Specific to the Provider API Schema Implications for JSON Schema or OpenAPI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants