-
Notifications
You must be signed in to change notification settings - Fork 81
cli: colorize build statuses #4078
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces colored output for build statuses using the rich library, which improves the user experience of the CLI. The implementation is mostly correct, adding a new --color/--no-color flag to control this feature. However, there's a critical compatibility issue with the argparse action used, as it's only available in Python 3.9+ while the project seems to support older versions like Python 3.6 on EPEL 8. I've also included a suggestion to improve code style and performance in the new helper function.
| parser.add_argument( | ||
| "--color", | ||
| action=argparse.BooleanOptionalAction, | ||
| default=True, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argparse.BooleanOptionalAction was introduced in Python 3.9. Since this project supports older Python versions (e.g., on EPEL8 which has Python 3.6), using this action will cause the program to fail on those systems. To maintain compatibility, you should use separate actions for enabling and disabling the color feature.
parser.add_argument('--color', dest='color', action='store_true', help="Enable colorized output (default).")
parser.add_argument('--no-color', dest='color', action='store_false', help="Disable colorized output.")
parser.set_defaults(color=True)
Pull Request validationFailed🔴 Failed or pending checks:
🔴 Review - Missing review from a member (2 required) |
Or we can package it. |
cli/copr_cli/main.py
Outdated
| parser.add_argument( | ||
| "--color", | ||
| action=argparse.BooleanOptionalAction, | ||
| default=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terminal colors are normally on by default, the frameworks detect incompatible terminals and automatically turn colors off... if something screws up, there should be --no-color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argparse.BooleanOptionalAction implements both --color and --no-color :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as Gemini pointed out, this is available only since EPEL9+ so we will probably need to do a mutually exclusive group or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, do we need the --color option?
| "failed": "red", | ||
| "succeeded": "green", | ||
| "canceled": "default", | ||
| "waiting": "default", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pity to keep canceled as "default".
Eventually, explicit "default" value doesn't make sense (it's returned if key is not found in dict).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colors are up to you, but those fields setting "default" to "default" are redundant, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but those fields setting "default" to "default" are redundant, right?
Correct, they are. I just wanted to enumerate the all of them to for easier searching and future changes. And I also wanted to be more explicit what their color is. But yea, they would be picked by the .get(..., "default") otherwise.
Good call, it wasn't that hard |
059aa96 to
0e24986
Compare
The `python-rich` package is available for EPEL9+ so for EPEL8 we would need to do some workaround.
|
The CI is still failing because the |


The
python-richpackage is available for EPEL9+ so for EPEL8 we would need to do some workaround.