Cylc Message Help Contains two patterns of use #5921
Conversation
|
Close, but the broken use case is still covered by the first usage, I think this does it: diff --git a/cylc/flow/scripts/message.py b/cylc/flow/scripts/message.py
index 061d842475..b12f0e180f 100755
--- a/cylc/flow/scripts/message.py
b/cylc/flow/scripts/message.py
@@ -109,8 109,9 @@ def get_option_parser() -> COP:
argdoc=[
WORKFLOW_ID_ARG_DOC,
('JOB', 'Job ID - CYCLE/TASK_NAME/SUBMIT_NUM'),
('[SEVERITY:]MESSAGE', 'Messages'),
COP.optional(
- ('[SEVERITY:]MESSAGE ...', 'Messages')
('[SEVERITY:]MESSAGE', 'Messages')
),
COP.LINEBREAK,
COP.optional(('[SEVERITY:]MESSAGE ...', 'Messages')), |
|
Whilst working with this branch checked out (as of last review) I got traceback for |
MetRonnie
requested review from
MetRonnie
and removed request for
markgrahamdawson
tests/functional/cli/04-kgo.t
Outdated
There was a problem hiding this comment.
Is this test really worthwhile? Might end up as a maintenance drag
There was a problem hiding this comment.
I semi agree. On the other hand, it's easy enough to regenerate the files. I wonder if it might be worth just testing
- cylc message
- cylc broadcast
- cylc tui
Which demonstrate both patterns in the code I've changed in this PR, with the benefit of adding a double check on the two scripts which we least want to drift. I added TUI to the list, because it seems to be an outlier. This test would have caught the errors in my PR.
Do you think that this is reasonable @MetRonnie ?
There was a problem hiding this comment.
Might be worth it for cylc message and cylc broadcast as these have to be stable for task scripts. Not sure why it would be worth it for TUI
There was a problem hiding this comment.
Seems a little non-standard - it broke the original PR>
There was a problem hiding this comment.
I don't get the point of this test?
All it's doing is checking whether any of the help text has changed?
It's not actually checking that the interfaces are documented correctly.
There was a problem hiding this comment.
The point is to check whether changes to the option parser have caused a change in the help text: My original changes caused unwanted changes - like the change to TUI.
Happy enough to remove if you really feel it's total overkill
wxtim
force-pushed
the
fix.broken_argdoc_cylc_message
branch
from
5c1147b
to
ad27da3
Compare
|
You might want to have a quick skim over |
I'm not sure what your point was here. As far as I can see the behaviour makes sense and is correct but the docs are wrong. What behaviour are you unhappy with? The behaviour described in #5883 looks exactly as I'd expect from a careful reading of @oliver-sanders - I'm not sure I remember what your objection to the way the app actually works was... |
might have multiple different valid arguments. e.g. Cylc message can be either cylc message -- [WORKFLOW JOB [[SEVERITY:]MESSAGE ...]]] __or__ cylc message -- [[SEVERITY:]MESSAGE ...]]] - Refactored part of the __init__ method of CylcOptionParser. into a method for ease of testing. - Added unit tests to get complete coverage of new method. Response to review: the ... from the second usage. Added extra information to the descriptions of the args. remove cli-help test
wxtim
force-pushed
the
fix.broken_argdoc_cylc_message
branch
from
ad27da3
to
1fd2de3
Compare
wxtim
changed the title
Closes #5883
A single script might have multiple different valid arguments.
for example
cylc messagecan be eitherThis PR fixes the Cylc message docs.
Note
The reason for this behaviour is given further down in the doc:
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).CHANGES.mdor in the docs.?.?.xbranch.