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

WIP: added text allowing "Z" for timezone offset. #586

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ChrisBarker-NOAA
Copy link
Contributor

@ChrisBarker-NOAA ChrisBarker-NOAA commented Jan 10, 2025

See issue #584 for discussion of these changes.

NOTE: I think there's consensus about allowing "Z" as a timezone offset (already allowed by udunits)

It was decided to include a "proper" recommendation that a zero or Z be included for UTC times -- but some text was added saying it is a good idea in some cases.

Release checklist

** Will do this once we've all agreed **

  • Authors updated in cf-conventions.adoc? Add in two places: on line 3 and under .Additional Authors in About the authors.
  • Next version in cf-conventions.adoc up to date? Versioning inspired by SemVer.
  • history.adoc up to date?
  • Conformance document up to date? -- no changes

For maintainers

After the merge remember to delete the source branch.
Tags are set at the conclusion of the annual meeting; until then, main always is a draft for the next version.

ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch07.adoc Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ChrisBarker-NOAA ChrisBarker-NOAA left a comment

Choose a reason for hiding this comment

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

I think I've addressed everything.

ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch04.adoc Outdated Show resolved Hide resolved
ch07.adoc Outdated Show resolved Hide resolved
@@ -330,7 +330,7 @@ variables:
ppn:cell_methods = "time: sum";
double time(time);
time:long_name = "time";
time:units = "h since 1998-4-19 6:0:0";
time:units = "h since 1998-4-19 6:0:0 Z";
Copy link
Contributor

@davidhassell davidhassell Jan 21, 2025

Choose a reason for hiding this comment

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

Edit - just seen the above conversion on this, so I retract the comment!

I found that the lack of space before Z is required: "If the time is in UTCm add a Z directly after the time without a space." (https://en.wikipedia.org/wiki/ISO_8601#Coordinated_Universal_Time_(UTC))

@ChrisBarker-NOAA
Copy link
Contributor Author

OK -- I think this is in good shape.

But I'm not sure what to do about:

  • Next version in cf-conventions.adoc up to date?

  • history.adoc up to date?

Also: I finally found a way to look at the rendered doc. Using "Z" as the placeholder, while also allowing the string "Z" is a bit confusing:

"or the letter "Z" are allowed in Z, not time zone names or acronyms."

the second Z is bold in the html, but a bit subtle.

I'm not going to change that now, but maybe in the future?

@JonathanGregory
Copy link
Contributor

Thanks, Chris.

We don't need a change to the conventions doc for this PR, so "N/A" or equivalent.

The issue should be added to the list for the working version in the revision history - however, the heading is not there. I think we first need a new working version to be created for CF 1.13, before this PR can be merged into it. David intends to do that.

@JonathanGregory JonathanGregory linked an issue Jan 22, 2025 that may be closed by this pull request
@JonathanGregory JonathanGregory added this to the 1.13 milestone Jan 22, 2025
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.

Add a recommendation to including a TZ offset in time units.
3 participants