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

Resolve some errors in the current master branch of CF 1.8 docs #238

Closed
mwengren opened this issue Jan 30, 2020 · 11 comments · Fixed by #240
Closed

Resolve some errors in the current master branch of CF 1.8 docs #238

mwengren opened this issue Jan 30, 2020 · 11 comments · Fixed by #240
Labels
defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors

Comments

@mwengren
Copy link
Contributor

Title

Resolve some errors in the current master branch of CF 1.8 docs

Moderator

@mwengren

Requirement Summary

This issue is simply a pointer to a PR to fix to some issues I found in new text added in Chapter 3.5 sometime since last release of the documentation. I separated these more minor fixes from my more in-depth PR on new QC flag documentation in Chapter 3.4 (#235) since they seem more likely to accepted on their own. #235 will require an update if these fixes are applied.

Technical Proposal Summary

Just a few simple fixes to consider before 1.8 version of the docs are released (or hopefully shortly afterwards n a patch release?). At the moment, the docs are a bit misleading due to paragraph and example mis-ordering.

Benefits

All readers of the CF docs

Status Quo

Just a minor fix of some documentation errors

Detailed Proposal

N/A

@mwengren mwengren added the defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors label Jan 30, 2020
@mwengren
Copy link
Contributor Author

I put this issue and accompanying PR together rather quickly at the end of the day yesterday, so apologies for the incompleteness. Re-reading the README again I see I can link more or less to the exact location in the draft version of the docs that I believe is incorrect (and PR #239 resolves):

http://cfconventions.org/cf-conventions/cf-conventions.html#region-variable-flag-masks-ex

  • The paragraph preceding this example belongs after it, and the paragraph following it belongs before, from what I can tell.
  • The 'List of Examples' also contains a duplicate link to Example 3.5 (intended to link to Example 3.4 instead)

Hopefully this helps illustrate this particular problem with the 1.8 docs as they stand now. Any chance of merging this before 1.8 is finalized?

@davidhassell
Copy link
Contributor

Hello @mwengren, I've had a quick look at the pull request and I'm afraid it wasn't obvious to me why the paragraph order should be changed - it seems natural to me to introduce flag_* attributes first and then go on to discuss a special case, which is what the current conventions do. Could you give a bit more of background?

Many thanks for your help,
David

@mwengren
Copy link
Contributor Author

mwengren commented Feb 8, 2020

@davidhassell Ah, yes, you are right. I had been paying attention to the introductory text to the examples, which did not flow. The problem was rather that the examples were out of order. Now Example 3.4 'A flag variable, using flag_masks' is properly cited immediately before, and Example 3.5 'A region variable, using flag_values' is also cited from the paragraph immediately preceding.

Had made these changes a little too quickly the first time around. If you read the draft 1.8 document linked above, you'll see where the examples are mis-ordered.

@mwengren
Copy link
Contributor Author

mwengren commented Feb 8, 2020

@davidhassell I just created a new PR #240 since the existing one had accumulated a messy commit history, please review that instead.

@zklaus
Copy link

zklaus commented Feb 10, 2020

@mwengren a good way to deal with this is rewriting history. In fact, it is very rare to find a good PR without rewritten history, imho.

@mwengren
Copy link
Contributor Author

Thanks @zklaus will give that a try next time, since I already made a replacement PR in this case.

#240 is good to go for this particular fix, please consider as the docs should make a lot more sense with the examples now in the correct order.

@mwengren
Copy link
Contributor Author

@davidhassell I think this issue of mis-ordered examples still exists in the beta 1.9 version of the docs. In interest of closing this out, can you (or some other gracious volunteer) review #240 and potentially merge?

I'd like to submit some additions to Chapter 3 related to our new standard names for quality flagging that were approved in #216, and getting this fixed will make that slightly easier. Thanks!

@mwengren
Copy link
Contributor Author

Ping... anyone? This relatively minor change hasn't received much commentary, and it's well past the three week threshold for review and merge. There is in fact an ordering issue with Example 3.4 and 3.5, as shown here:

http://cfconventions.org/cf-conventions/cf-conventions.html#region-variable-flag-masks-ex

(notice that the text preceding each example actually refers to the other of the two examples whose order I swapped in PR #240).

Would love to close this out. Many thanks.

@JonathanGregory
Copy link
Contributor

This looks OK to me. You have swapped the contents of the examples to make them fit the text - is that right?

@mwengren
Copy link
Contributor Author

mwengren commented Apr 1, 2020

@JonathanGregory Yes! And fixed a dead link in the table of contents. Simple stuff. Thanks for reviewing!

@JonathanGregory
Copy link
Contributor

I've merged it. As you say, it was past the time limit and since it was a defect it's approved by default. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors
Projects
None yet
4 participants