Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Nov 27, 2024

color-dispatch

@Marenz Marenz requested review from a team as code owners November 27, 2024 17:04
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:cli Affects the command-line interface part:dispatcher labels Nov 27, 2024
@github-actions github-actions bot added the part:docs Affects the documentation label Nov 27, 2024
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

Awesome! It might also be a good idea to maybe set constants with colors, so they are easier to tune if needed, like COLOR_DISPATCH_STARTED, VALUE_NONE_COLOR, COLOR_VALUE_TRUE, etc.

But all comments optional.

"""Print the dispatch details in a nicely formatted way with colors."""
# Determine the status and color
status: str = "running" if dispatch.started else "not running"
status_color: str = "green" if dispatch.started else "red"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: What about gray for not started? Red looks like an error to me. Maybe we can sort of mimic systemctl status as many users may be familiar with them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My desire was that it stands out immediately what the status is :)

val_color = "white"

if value in ("None", "False"):
val_color = "red"
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here maybe gray for None?

required=True,
)
@click.option(
"--raw",
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible future improvements:

  • Split --raw from --no-color
  • Set color on/off automatically based on sys.stdin.isatty()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set color on/off automatically based on sys.stdin.isatty()

that actually seems to happen automatically, at least there is no color if I pipe it through cat or forward it into a file

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool. Extra point for click 💯

@Marenz Marenz added this pull request to the merge queue Nov 28, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit 108ca25 Nov 28, 2024
14 checks passed
@Marenz Marenz deleted the fancy branch November 28, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:cli Affects the command-line interface part:dispatcher part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants