-
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
#981: final cycle specification as an offset. #1055
#981: final cycle specification as an offset. #1055
Conversation
@benfitzpatrick - please review. |
def _coerce_final_cycletime( value, keys, args ): | ||
"""Coerce final cycle point.""" | ||
value = _strip_and_unquote( keys, value ) | ||
if "P" in str(value): |
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.
Would a value of T06
pass this coercer?
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.
No - should it? I'm not seeing any failures of existing examples of this in the test battery and I've only been working in terms of +P, as outlined in the documentation changes.
@benfitzpatrick - feedback addressed, see the above query/comment re. T06. |
Hold off a moment, odd behaviour from the test battery with the feedback changes. |
We should be able to use expressions like this. |
#1044 is a separate issue though isn't it as it applies to not just this case? |
…point Conflicts: lib/cylc/config.py
@benfitzpatrick - done. T06 now covered (and in test battery). Had to correct a reload test as it hadn't been converted fully to the new syntax. |
def _coerce_final_cycletime( value, keys, args ): | ||
"""Coerce final cycle point.""" | ||
value = _strip_and_unquote( keys, value ) | ||
if "P" in str(value) or str(value).startswith("T"): |
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.
Unfortunately, this won't pick up values like 01T
.
Maybe we just need to avoid testing final cycle point validity here - i.e.
remove this function entirely.
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.
As discussed I'll strip it down to a bare minimum so we can put back in better functionality once we know what we're doing.
@benfitzpatrick - done. This should cover everything discussed. |
Looks good, works. I'm taking this without the final go-ahead from Ben as it is a small change, and I want to get everything on to master before any more pull requests come in. |
#981: final cycle specification as an offset.
Closes #981