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

CLI: don't color if stdout redirected. #4562

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Dec 22, 2021

These changes close #4552 and supersede #4559

Current colour output disabling is a bit broken.

On master, this correctly generates colour output in the terminal:

$ cylc cat-log test 

But these cases (for example) get unnecessary color-code pollution:

$ cylc cat-log test | vim -   # IN YOUR FACE emacs!!! (can't read from stdin)
$ cylc cat-log test > log

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).
  • Already covered by existing tests.
  • Does not need tests (why?).
  • Appropriate change log entry included.

  • No change log entry required (why? e.g. invisible to users).

  • No documentation update required.

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Makes sense as a change.
  • Works with the test
    $ cylc validate simplest > log
    $ cat -A log
    # On master:
    ^[[32mValid for cylc-8.0rc1.dev^[[0m^[[0m$\n^[[0m
    # On branch
    Valid for cylc-8.0rc1.dev

RE Testing - I spent a good deal of time yesterday considering how to test this, and struggled. I'm happy to manually test this.

Question: I'm happy with manually testing things, but might it be worth having a document somewhere listing and explaining manual tests.

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.

Yeah, I think this is sensible, why would you want to pipe colour?

Also reading from stdin is so dammed useful.

@wxtim wxtim merged commit d1dedac into cylc:master Jan 4, 2022
@oliver-sanders
Copy link
Member

@wxtim this was waiting on the unchecked bullets in the description.

@hjoliver
Copy link
Member Author

hjoliver commented Jan 5, 2022

No worries, I'll post a follow-up.

@hjoliver hjoliver deleted the terminal-colour-fix branch January 5, 2022 05:04
@hjoliver hjoliver mentioned this pull request Jan 5, 2022
9 tasks
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.

Scripts should turn colour off in non-interactive shells
3 participants