-
Notifications
You must be signed in to change notification settings - Fork 95
Ensure next/prev syntax works for --startcp option
#7024
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
base: 8.6.x
Are you sure you want to change the base?
Conversation
Is this not adding support for the next/prev syntax here? |
|
Yes, it seems to be an odd omission seeing as it works for the initial cycle point option |
|
Er, it was only ever intended / documented to work at the ICP, but I can see why it might be useful for the start cycle (though I don't think opps would want to specify the warm cycle point implicitly like this?). Plz can you change from "fix" to "feat". |
Warm start from 00Z, 12Z etc? This is why I implemented it! |
| "Set the start cycle point, which may be after" | ||
| " the initial cycle point. If the specified start point is" | ||
| " not in the sequence, the next on-sequence point will" | ||
| " be used. (Not to be confused with the initial cycle point)"), |
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.
(I notice leading spaces have crept into the codebase - not sure what the motivation is, as this is not how we write paragraphs ordinarily. Perhaps we should specify the style to follow to avoid inconsistency?)
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.
???
Whether the space goes at the start or end of the line is arbitrary, neither is "how we write paragraphs ordinarily" since we don't write this whitespace in plain text. So neither way is right or wrong, natural or unnatural, it's just a style choice like any other.
Leading spaces are preferable because it is much easier to spot when a space has been omitted as they are all aligned in the first column (we have had several cases where tailing spaces were omitted) and with this arrangement commenting out or removing a line will never leave an erroneous space.
It's exactly the same reason why placing operators at the start rather than end of the line is considered preferable (PEP8, flake8, black, ruff, prettier).
FYI: There are no linting rules for strings, so style cannot be enforced.
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.
I agree with @oliver-sanders
oliver-sanders
left a comment
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 but can't test ATM.
| @@ -0,0 +1 @@ | |||
| Ensure `next()/previous()` syntax works for the `cylc play --startcp` option. | |||
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.
| Ensure `next()/previous()` syntax works for the `cylc play --startcp` option. | |
| Add support for the `next()/previous()` syntax with the `cylc play --startcp` option. |
| "Set the start cycle point, which may be after" | ||
| " the initial cycle point. If the specified start point is" | ||
| " not in the sequence, the next on-sequence point will" | ||
| " be used. (Not to be confused with the initial cycle point)"), |
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.
???
Whether the space goes at the start or end of the line is arbitrary, neither is "how we write paragraphs ordinarily" since we don't write this whitespace in plain text. So neither way is right or wrong, natural or unnatural, it's just a style choice like any other.
Leading spaces are preferable because it is much easier to spot when a space has been omitted as they are all aligned in the first column (we have had several cases where tailing spaces were omitted) and with this arrangement commenting out or removing a line will never leave an erroneous space.
It's exactly the same reason why placing operators at the start rather than end of the line is considered preferable (PEP8, flake8, black, ruff, prettier).
FYI: There are no linting rules for strings, so style cannot be enforced.
| "Set the initial cycle point. " | ||
| "Required if not defined in flow.cylc.\n" | ||
| "Can be an absolute point or an offset relative to the " | ||
| "current time - see " | ||
| f"{SHORTLINK_TO_ICP_DOCS} (Cylc documentation link)." |
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.
Why change this?
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.
For clarity
| "Set the start cycle point, which may be after " | ||
| "the initial cycle point. " | ||
| "If the specified start point is not in the sequence, the next " | ||
| "on-sequence point will be used.\n" | ||
| "Can be an absolute point or an offset relative to the " | ||
| "current time, like the --initial-cycle-point option - see " | ||
| f"{SHORTLINK_TO_ICP_DOCS} (Cylc documentation link)." |
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.
Why change this?
|
Looks LGTM, and seemed to work when I tried it out, apart from a bug where Cylc VIP or Play seems to freeze if the startcp >> icp because checking whether it is in the sequence(s) can take a long time if there are lots of items in the sequence. |
|
(unrelated issue) |
|
@wxtim are you going to approve? |
| "Set the start cycle point, which may be after" | ||
| " the initial cycle point. If the specified start point is" | ||
| " not in the sequence, the next on-sequence point will" | ||
| " be used. (Not to be confused with the initial cycle point)"), |
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.
I agree with @oliver-sanders
Sort of - I failed to spot that I had pending comments - I agree with Oliver about where spacing goes, and even if I didn't I'd rather not have changes like that in this PR. Technically, I think it works. 👍🏼 |
|
I haven't changed the whitespace just for the sake of it, it was along with a bit of a rewrite for clarity. Happy to change the whitespace back although it sounded like there was no consensus on it (and I can't say I am compelled by the argument for leading whitespace within strings like that, I find it a bit more difficult to read that way). |
E.g.
cylc play --startcp 'previous(T00)'Check List
CONTRIBUTING.mdand added my name as a Code Contributor.?.?.xbranch.