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

Tweak terminal colour handling. #4568

Merged
merged 8 commits into from
Jan 25, 2022
Merged

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jan 5, 2022

Follow-up to #4562

  • strip colorama markup from command help if not needed
  • strip hardwired ansimarkup tags from command help if not needed (cylc scan --help)

Requirements 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.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@hjoliver hjoliver added the bug Something is wrong :( label Jan 6, 2022
@hjoliver hjoliver self-assigned this Jan 6, 2022
@hjoliver hjoliver added this to the cylc-8.0rc1 milestone Jan 6, 2022
@hjoliver hjoliver marked this pull request as ready for review January 6, 2022 09:26
@hjoliver
Copy link
Member Author

hjoliver commented Jan 6, 2022

Note, it's kind of annoying that we have to treat command help separately for color switching (because it is printed by the option parser during option parsing) but I don't think it matters much. (There are more important things to think about right now).

@hjoliver
Copy link
Member Author

hjoliver commented Jan 6, 2022

(Assigned reviewers from the precursor PR)

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved
cylc/flow/option_parsers.py Outdated Show resolved Hide resolved
@hjoliver hjoliver force-pushed the terminal-colour-fix-2 branch 3 times, most recently from 3728f4e to 219782f Compare January 7, 2022 00:43
@hjoliver
Copy link
Member Author

hjoliver commented Jan 7, 2022

@oliver-sanders - I've gone with a hybrid approach: a help formatter that uses my crude sys.argv parsing to get at the color options, because as I noted above help formatters do not have access to parsed option values. (I suppose that makes sense: help is often printed in response to errors on the command line).

This is not really any different than what I had before, except that the help formatter is formally nicer than overriding parser.print_help() 😁 However, the internal logic is robust enough (not fragile) it seems to me.

@oliver-sanders
Copy link
Member

help formatters do not have access to parsed option values

That didn't make a lick of sense to me since the CLI args have to be parsed in order for the parser to activate the help formatter.

I think I've found the problem, if the --help argument was used any other args were stripped from the invoked command causing the strange dual state where the args passed to the parser did not match the args in sys.argv.

I think this change should fix the problem making args available to the help formatter.

diff --git a/cylc/flow/scripts/cylc.py b/cylc/flow/scripts/cylc.py
index 036cf4c05..6fa753315 100644
--- a/cylc/flow/scripts/cylc.py
+++ b/cylc/flow/scripts/cylc.py
@@ -532,7 +532,7 @@ def main():
                 # check if this is a command abbreviation or exit
                 command = match_command(command)
             if opts.help_:
-                execute_cmd(command, "--help")
+                execute_cmd(command, *cmd_args, "--help")
             else:
                 if opts.version:
                     cmd_args.append("--version")

MB!

@hjoliver
Copy link
Member Author

I think I've found the problem, if the --help argument was used any other args were stripped from the invoked command causing the strange dual state where the args passed to the parser did not match the args in sys.argv.

OK, brilliant, well done 👍

@hjoliver
Copy link
Member Author

Over-confident declaration of rightness retracted on Element chat!

@hjoliver hjoliver force-pushed the terminal-colour-fix-2 branch 2 times, most recently from f39dbe5 to 7ce3e31 Compare January 18, 2022 02:04
@hjoliver
Copy link
Member Author

hjoliver commented Jan 18, 2022

I came back to this late in the day (out of time) I'm not sure why the new unit tests are failing. Monkeypatching of sys.argv appears to have worked (e.g. put print(sys.argv) in there) but is not affecting command parsing in the tests. I must have something wrong... [Fixed]

@hjoliver hjoliver force-pushed the terminal-colour-fix-2 branch from 7ce3e31 to 6620182 Compare January 18, 2022 03:42
@oliver-sanders
Copy link
Member

Tests timed out, poking them to see if it is reproducible.

@oliver-sanders
Copy link
Member

... verily verily, not sure why.

@oliver-sanders
Copy link
Member

Lets see if a merge sorts things out...

@hjoliver hjoliver force-pushed the terminal-colour-fix-2 branch from b9ec819 to 46bb26a Compare January 25, 2022 01:47
@hjoliver
Copy link
Member Author

Minor tweak, should be good to go now.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

🌈

@oliver-sanders
Copy link
Member

(maybe squash)

@wxtim wxtim merged commit c776ce3 into cylc:master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants