-
Notifications
You must be signed in to change notification settings - Fork 94
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
minor fix to docs about exit-script #3066
Conversation
@ColemanTom - thanks, good spotting. These sections (and the "See also" lists) are more or less un-ordered (probably due to organic evolution of the docs over time). But you are right, they should be in the right order. The right order is as shown in this diagram: #2962 (comment) (only one of exit-script or err-script gets run, depending on exit status) Would you like to amend this PR with changes to order these correctly? |
(Also, when you're done, could you cherry-pick your changes to another new PR for the 7.8.x branch - thanks) |
K. One clarification first though, https://github.com/cylc/cylc/pull/3066/files#diff-8838fd940264b809b57525f23c2137deR1370 There is a |
I'm guessing that's some kind of typo (seems to be the only occurrence) but should maybe let @sadielbartholomew comment (I'm not too familiar with the new documentation format yet). |
Chiming in. I did a Probably safe to remove IMO, if you need to. Just saw @hjoliver 's comment. +1 for passing to @sadielbartholomew |
init, env, pre, post, err, exit, and script have been adjusted to be displayed in the docs in chronological order.
88dbf73
to
ffb19cc
Compare
I've updated the orders, and removed that I'll cherry-pick once this has been approved in case more changes are required.. |
Thanks @ColemanTom 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; will approve when the back-port PR goes up.
Back port PR is #3070? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind the change is good, the documentation still builds & is still formatted correctly, &:
There is a .. _ScriptItem just above the script section. I can't see that used anywhere, so is it ok for me to delete it, or is there a missing ref somewhere else in the documentation?
I'm guessing that's some kind of typo (seems to be the only occurrence) but should maybe let @sadielbartholomew comment
no that is not meant to be there & should rightly be removed. Well spotted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ToDo comment deleted here too, as for the 7.8.x branch)
I noticed that the
exit-script
section in the docs referred to itself rather thanerr-script
, so this just adjusts that.I did contemplate another adjustment. In the order of the docs, it has
exit-script
aboveerr-script
. But in theSee also ...
parts for these script sections, it listserr-script
beforeexit-script
. I'm not sure what the ordering in here is meant to be, but I would suggest that it should be order of execution really for both - it makes the documentation more intuitive.