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

Single-source the version number #343

Closed
adigitoleo opened this issue Jan 2, 2022 · 24 comments · Fixed by #344
Closed

Single-source the version number #343

adigitoleo opened this issue Jan 2, 2022 · 24 comments · Fixed by #344
Assignees
Labels
change agreed Issue accepted for inclusion in the next version and closed enhancement Proposals to add new capabilities, improve existing ones in the conventions, improve style or format

Comments

@adigitoleo
Copy link

adigitoleo commented Jan 2, 2022

The current published standard (v1.9) contains a typo in section 1.4 Overview:

Files using this version of the CF Conventions must set the NUG defined attribute Conventions to contain the string value "CF-1.8" to identify datasets that conform to these conventions.

So, the version number was not incremented in that paragraph. This could be avoided by somehow reading the version number from a central location when building the document, if that is possible.

Just something I noticed when reading through again today.

6 Jan 2022. I'm adding the enhancement label, because we are considering #344 as a solution, and that's a substantive change. @JonathanGregory

@adigitoleo adigitoleo added the typo label Jan 2, 2022
@zklaus
Copy link

zklaus commented Jan 2, 2022

This should be possible using a "replacement" as described in Section 26.9 of the asciidoc userguide.

@erget
Copy link
Member

erget commented Jan 3, 2022

Hi all, so it looks like we actually have 2 things in this issue:

  1. Fix typo (easy)
  2. Potentially improve the document so it's more future-proof

(1) sounds necessary to me; (2) sounds like a good idea. Is anybody willing to champion 1 or both of these? I would support it!

@JonathanGregory
Copy link
Contributor

Dear all

Thanks for pointing it out, Leon @adigitoleo. I think @zklaus is right that we could add a "replacement" i.e. a sort of macro in AsciiDoc for the current version number, so that only one occurrence needed to be changed. I am not familiar with the build process so I won't volunteer to do that. Is there a checklist for making a new version, to which this could be added? I think @davidhassell must know. As Daniel @erget says, it would be easy to fix the typo, and I'm sure we would approve the change as a defect issue for it . But could we agree to modify the live 1.9 document immediately? There would be no point in agreeing a defect correction to 1.9 which wasn't going live until 1.10 in this case!

Best wishes and happy 2022 to the CF community

Jonathan

@zklaus zklaus mentioned this issue Jan 4, 2022
4 tasks
@zklaus
Copy link

zklaus commented Jan 4, 2022

I added a draft PR in #344 that addresses (2). If we decide to tackle (1) separately, the list of changes in that PR should give a good overview of the places that need changing.

@erget
Copy link
Member

erget commented Jan 5, 2022

Thanks @zklaus, the PR looks pretty cool.

Also yes, happy new year everybody! :)

I've requested also the input of @davidhassell since he's more heavily involved in the release process. @JonathanGregory, I propose that we proceed as follows:

  • If we can agree on an approach for a longer-term fix, e.g. by accepting the changes proposed in Single source version #344, then we cherry-pick those on top of 1.9 and make a patch release to correct the defect.
  • If the long-term approach takes longer to clarify, i.e. more than a week or so, we manually correct the defect in 1.9.

What do you think?
Cheers,
Daniel

@JonathanGregory
Copy link
Contributor

Dear @zlaus, @erget, @davidhassell et al.

Thanks for the PR, Klaus. I agree that you've provided a neat solution, as Daniel says in other words.

I agree with renaming the top-level anchor of the conformance document. It's probably more useful if generic anyway.

I note that you have helpfully made this change in an example in ch7. Is this the only example in the whole document which includes the Conventions attribute? If so, it would be easier to delete it in that example, and that would also be better for consistency.

Daniel suggests defining current-version to 1.10 draft, not just 1.10, until it's released. I think that's a good idea. It means that draft would automatically appear in the titles of both conventions and conformance documents. This procedure should be added to the release checklist, if there is one.

Is there a reason why we should not release 1.10 with this change as soon as we've agreed it, and any others agreed since 1.9, and then start preparing 1.11 for release in the summer?

Cheers

Jonathan

@adigitoleo
Copy link
Author

adigitoleo commented Jan 5, 2022

@JonathanGregory I see the typo in the following places:

  • Section 1.4, par 2
  • Section 2.6.1 par 1
  • Section 7.5 in the example

There are no other examples with the Convention attribute. Looking at the built artifacts from that PR, there are no more version number typos that I can see.

@JonathanGregory JonathanGregory added the enhancement Proposals to add new capabilities, improve existing ones in the conventions, improve style or format label Jan 6, 2022
@JonathanGregory
Copy link
Contributor

Discussion in review comments of #344 reproduced here for the record. (May I politely and respectfully remind everyone that the CF practice is to keep substantive discussions about text in the issue, not the PR, so we have a single place to look for the history. Thanks. 😄 )

@erget:

Might it make sense to do this?:

:current-version: 1.10 draft

And then remove draft pre-release?

@zklaus:

I had this locally at some point, but I removed it again because I felt that it looked odd in the :Conventions attribute both in the examples, which would read

// global attributes:
  :Conventions = "CF-1.10 draft" ;
  :featureType = "timeSeries" ;

and in the text where this attribute is described, for example

[...] attribute **`Conventions`** to a string value that contains "**`CF-1.10 draft`**".

Hence my suggestion to add a second :status-indicator: or similar that can be combined with the :current-version: where appropriate.

But I am happy to use the simpler solution :)

@erget:

I don't have overly strong feelings here, but in the absence of Sturm und Drang from other corners my preference would be to go with using current-version for the whole shebang. Reasoning:

  • It's simpler than having 2 attributes
  • It doesn't result in invalid CDL, since space characters are allowed

and thus it would be easier to maintain, and also more obvious that one is working with a draft.

@erget
Copy link
Member

erget commented Jan 6, 2022

@JonathanGregory thanks for bringing us back on track!

Might it make sense to do this?:

:current-version: 1.10 draft

And then remove draft pre-release?

I think that would make sense.

@JonathanGregory
Copy link
Contributor

I agree with putting draft in the current-version. I think it would be a positive advantage if it turns up e.g. in Conventions (in the text of the draft document), because that makes the status obvious. In fact maybe we could be more explicit and say e.g. :current-version: 1.10 draft (not yet released, not to be used) or is that going too far?

@erget
Copy link
Member

erget commented Jan 6, 2022

Personally, I'm more for the terser formulation of draft - IMO that makes it obvious enough.

@ethanrd
Copy link
Member

ethanrd commented Jan 6, 2022

I prefer the more terse formulation. Replacing the space with a dash, so 1.10-draft, would be similar to a well known pattern for software versioning of appending -SNAPSHOT or -yyyymmddhhss to a version number. Perhaps a bit more obvious than 1.10 draft but still terse.

@erget erget linked a pull request Jan 7, 2022 that will close this issue
4 tasks
@erget
Copy link
Member

erget commented Jan 7, 2022

From @zklaus in #344 (comment):

I have added a fancified version of the version handling. Let me know what you think or if you want me to explain a bit more.

PS: I am totally happy to roll back the last commit and to just add draft or -draft to the version. Just thought I'd show one other way that we could go about this to improve automation.

I think this is pretty elegant - IMO in order to make this perfect the way to go would be to change the build workflow to run with -a final as described here on full tags - that sounds straightforward but I'm not sure if that's easy to do though, as I don't have a clear pathway to do that in mind. Any thoughts on that, and would anybody be interested in some extra credit for improving the build process? ;)

@JonathanGregory
Copy link
Contributor

That's clever, @zklaus. Thanks. I agree with @erget, and I am not an expert on the build workflow.

@JonathanGregory
Copy link
Contributor

Dear @zklaus

As @adigitoleo says, there are no examples in the document which contain the Conventions attribute except in sect 7.5 (examples 7.15 and 7.16). Therefore I'd suggest you delete the Conventions attribute in those two examples, instead of correcting it. It's not necessary (now that 7.5 is an established part of the conventions).

Best wishes

Jonathan

@ethanrd
Copy link
Member

ethanrd commented Jan 7, 2022

It took me awhile to understand the meaning of the term attribute-version. Perhaps current-version-as-attribute?

I like the definitive nature of updating the version from 1.10-draft to 1.10 for release and then immediately updating to 1.11-draft. It leaves an artifact (source zip file) that is clearly marked as a full release. On the other hand, this is a documentation project not a software project and we distribute document artifacts not source artifacts. However, it might be more of an issue depending on where the CF DOI conversation about version specific DOIs goes.

@zklaus
Copy link

zklaus commented Jan 7, 2022

Re workflow, that was exactly my thinking. I have added this now to #344.

Re removing the attribute from the examples, I am not so sure. I think we should probably consider categorizing examples in the conventions as either "full examples" or "simple examples"/"excerpts" and then rather add the attribute to all full examples and possibly some excerpts as appropriate. My reasoning is that I think many first time producers of data files will follow closely some examples and if we leave out this attribute, they might too.
But perhaps this deserves a separate discussion.

Re @ethanrd's comment on the naming of the attribute, I completely agree, attribute-version is not very clear. How about current-version-in-attribute or current-version-for-attribute?

@JonathanGregory
Copy link
Contributor

Dear @zklaus

I don't think there are any "full examples" in the document. (This new issue is relevant to this.) That is probably because full examples are not what we need to illustrate particular points. I will propose the deletion of the Conventions attribute in this sole example as a separate issue.

Best wishes

Jonathan

@zklaus
Copy link

zklaus commented Jan 10, 2022

Thanks, @JonathanGregory, your points make sense to me. Let's take that discussion in the other issue and move forward here with the corrected attributes in place.

@JonathanGregory
Copy link
Contributor

Dear @zklaus

I've done a pull request to delete the Conventions attribute in the two examples, as proposed. I suppose that might lead to a clash when merging, if your change updates the attributes, would it? I am not a git expert (as is well-known).

Best wishes

Jonathan

@zklaus
Copy link

zklaus commented Jan 11, 2022

Dear @JonathanGregory,

such a clash is called a conflict in git lingo and indeed does occur. But it is also one of the most common problems in versioning and as such one of the core abilities and raison d'etre of git is to help with their resolution.
So don't worry, this will be easy to address.

Cheers
Klaus

@JonathanGregory
Copy link
Contributor

Thanks, @zklaus. The example has been deleted now by #349, thanks to @erget for merging the pull request.

@zklaus
Copy link

zklaus commented Feb 1, 2022

Thanks for the heads-up, @JonathanGregory. I have updated #344 accordingly.

I have also addressed @ethanrd's suggestion of making the attribute name more explicit.
Given the general support and the fact that no more comments seem to be outstanding, I think we can start the clock and merge the PR in three weeks, on 2022-02-22.

@erget erget self-assigned this Feb 1, 2022
@erget
Copy link
Member

erget commented Feb 1, 2022

Reminder set. If you have a problem, speak in the next 3w or forever hold your peace!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change agreed Issue accepted for inclusion in the next version and closed enhancement Proposals to add new capabilities, improve existing ones in the conventions, improve style or format
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants