aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
412 stars 184 forks source link

CLI: Add color flag to `verdi process list` #6434

Open agoscinski opened 1 month ago

agoscinski commented 1 month ago

Adds the flag --color/--no-color to verdi process list that colors the process state if on (default is on). Implements feature request #4955.

In principle it is not hard to make the color and symbols configurable because we could pass the config from the context ctx.obj.config. To not pollute the config with a bunch of styling variables, I would only use one variable that is referring to a separate config file with all styling variables. EDIT: (But I think there are more important things to work on for now, but I would make a new issue about it.)

I thought about doing the option like the ls command --color=always|auto|never, it adds the option to enforce the color also when piping, so in ls -l > out the out file would have the color strings. But I never have found this option very understandable. Right now it works like --color = auto and --no-color = never.

I am actually not sure how to test this meaningfully.

EDIT: the color scheme is like in the issue and also agrees with the color scheme in the plumpy doc https://plumpy.readthedocs.io/en/latest/concepts.html#process (I gu

I use click for styling since we use it for all CLI.

danielhollas commented 1 month ago

Exciting! :rainbow:

I thoutght about doing the option like the ls command --color=always|auto|never, it adds the option to enforce the color also when piping, so in ls -l > out the out file would have the color strings. But I never have found this option very understandable. Right now it works like --color = auto and --no-color = never.

I feel quite strongly that the default behaviour should be auto i.e. you should somehow detect whether we're in tty mode, otherwise this new default behaviour will break users who pipe its stdout into another command.

Ideally, we'd also respect FORCE_COLOR and NO_COLOR environment variables. https://force-color.org/ https://no-color.org/

(this is e.g. how Python 3.13 will handle colors in tracebacks as well)

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 63.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 77.82%. Comparing base (ef60b66) to head (7187813). Report is 39 commits behind head on main.

Files Patch % Lines
src/aiida/__main__.py 0.00% 9 Missing :warning:
src/aiida/tools/query/formatting.py 88.24% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6434 +/- ## ========================================== + Coverage 77.51% 77.82% +0.32% ========================================== Files 560 561 +1 Lines 41444 41790 +346 ========================================== + Hits 32120 32519 +399 + Misses 9324 9271 -53 ``` | [Flag](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6434/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | Coverage Δ | | |---|---|---| | [presto](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6434/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | `73.14% <63.34%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

agoscinski commented 1 week ago

I put the logic for the color and symbols of process states into aiida/common, but I am not sure maybe aiida/cmdline is a better place for it as it only effects the styling for the cmdline? But technically src/aiida/tools/query/formatting.py can be also used outside of the CLI.