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

Cylc Message Help Contains two patterns of use #5921

Draft
wants to merge 1 commit into
base: 8.3.x
Choose a base branch
from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jan 12, 2024

Closes #5883

A single script might have multiple different valid arguments.

for example cylc message can be either

cylc message -- [WORKFLOW JOB [[SEVERITY:]MESSAGE ...]]] __or__
cylc message --	[[SEVERITY:]MESSAGE ...]]]

This PR fixes the Cylc message docs.

  • 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.

Note

The reason for this behaviour is given further down in the doc:

For backward compatibility, if number of arguments is less than or equal to 2,
the command assumes the classic interface, where all arguments are messages.
Otherwise, the first 2 arguments are assumed to be workflow ID and job
identifier.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • This is effectively a documentation change and doesn't need logging in CHANGES.md or in the docs.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim self-assigned this Jan 16, 2024
@wxtim wxtim added this to the cylc-8.2.5 milestone Jan 16, 2024
@wxtim wxtim linked an issue Jan 16, 2024 that may be closed by this pull request
@oliver-sanders
Copy link
Member

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')),

@oliver-sanders
Copy link
Member

Whilst working with this branch checked out (as of last review) I got traceback for cylc tui <workflow>/<run1>.

@MetRonnie MetRonnie requested review from MetRonnie and removed request for markgrahamdawson January 17, 2024 16:27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test really worthwhile? Might end up as a maintenance drag

Copy link
Member Author

@wxtim wxtim Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@MetRonnie MetRonnie Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a little non-standard - it broke the original PR>

Copy link
Member

@oliver-sanders oliver-sanders Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@wxtim wxtim Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 wxtim requested a review from MetRonnie January 18, 2024 08:57
@wxtim wxtim force-pushed the fix.broken_argdoc_cylc_message branch from 5c1147b to ad27da3 Compare January 18, 2024 17:06
@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 22, 2024

You might want to have a quick skim over cylc message --help bearing in mind #5883...

@wxtim wxtim marked this pull request as draft January 23, 2024 10:15
@wxtim
Copy link
Member Author

wxtim commented Feb 20, 2024

You might want to have a quick skim over cylc message --help bearing in mind #5883...

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 --help.

@oliver-sanders - I'm not sure I remember what your objection to the way the app actually works was...

@oliver-sanders oliver-sanders modified the milestones: cylc-8.2.5, 8.2.x Feb 22, 2024
@MetRonnie MetRonnie modified the milestones: 8.2.x, 8.3.x May 21, 2024
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 wxtim force-pushed the fix.broken_argdoc_cylc_message branch from ad27da3 to 1fd2de3 Compare October 4, 2024 14:51
@wxtim wxtim changed the base branch from 8.2.x to 8.3.x October 4, 2024 14:51
@wxtim wxtim changed the title Allow CylcOptionParser to handle >1 valid use cases. Cylc Message Help Contains two patterns of use Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc message: three or more outputs cause problems
3 participants