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 Temporal RS definition #72

Merged
merged 2 commits into from
May 18, 2022
Merged

Clarify Temporal RS definition #72

merged 2 commits into from
May 18, 2022

Conversation

jonblower
Copy link
Contributor

Addresses #60

@jonblower jonblower requested a review from chris-little May 12, 2022 14:50
@jonblower
Copy link
Contributor Author

The substantive change here is that the spec now makes the ISO8601-based string representation mandatory for the common case of using TemporalRS with a Gregorian calendar. The intention is to make it easier to develop clients that handle this common case, whilst not precluding the possibility of allowing other representations in future.

@jonblower
Copy link
Contributor Author

Would benefit from a review from @letmaik too

Copy link
Contributor

@letmaik letmaik left a comment

Choose a reason for hiding this comment

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

Looks good, I think this is focused enough to not cause breaks in the next version.

Copy link
Contributor

@chris-little chris-little left a comment

Choose a reason for hiding this comment

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

looks good.

@chris-little
Copy link
Contributor

@letmaik @jonblower Are you happy to merge now, or would you rather wait until the team meeting 2022-05-18 to give others a chance to consider?

@jonblower
Copy link
Contributor Author

Ideally it would be great to have input from @marqh with a view to merging early next week if poss? If Mark's not available then maybe we could just give it a final sanity check in the team meeting so people have had a chance to see it.

@chris-little
Copy link
Contributor

@marqh Are you happy with the CoverageJSON backward-compatible compromise wording? Recognising that we will probably do a larger scale alignment of temporal things in the next versions.

Copy link
Member

@marqh marqh 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 a sensible update to me.
one slight query, but i approve

@@ -338,24 +338,23 @@ Example of a vertical CRS, here representing height above the NAV88 datum:

### 5.2. Temporal Reference Systems

Time is referenced by a temporal reference system (temporal RS).
In this specification, only a string-based notation for time values is defined.
Time is referenced by a temporal reference system (temporal RS). In the current specification, only a string-based notation for time values is defined. Future versions of this specification may allow for alternative notations, such as recording time values as numeric offsets from a given temporal datum (e.g. "days since 1970-01-01").
Copy link
Member

Choose a reason for hiding this comment

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

i don't object to this extra sentence, but i'm a little surprised to see a 'future may' included in this section of the specification.
if it's felt to be of value, then please disregard this comment, but i'd slightly stick to the original and leave this as implied, rather than state a conditional future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, and I wondered that too. I added that sentence as it's frequently asked by users, but it could be that the sentence doesn't really belong in a specification, but in some kind of FAQ or guide. Maybe we can discuss on the next call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Iagree to leave in the infromative text, as itmakes it easier to read. the text style is really closer to W3C documents rather than ISO

@chris-little chris-little merged commit 806e382 into master May 18, 2022
@chris-little chris-little deleted the clarify-temporal-rs branch May 18, 2022 14:23
@chris-little
Copy link
Contributor

agreed at 2022-05-18 meeting

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.

4 participants