-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-139946: distinguish stdout or stderr when colorizing output in argparse #140495
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
gh-139946: distinguish stdout or stderr when colorizing output in argparse #140495
Conversation
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Thanks for addressing the comments. After thinking about this more, I'm a bit concerned about API compatibility. Adding the formatter parameter to Also, we'll need to add some tests here, since this wasn't caught with our existing test suite. |
After originally adding colour to argparse (#132323), we did get reports (#133653) that subclassing TypeError: HelpFormatter.__init__() got an unexpected keyword argument 'prefix_chars'We initially considered this a docs issue, because We could again consider this a docs issue, but I think we need to be careful about backwards compatibility especially after 3.14.0. Could be worth checking things like tox, virtualenv and mypy that were affected last time. And if we choose not to backport, we can still use ❯ NO_COLOR=1 ./python.exe -m calendar 2020 01 01 2> err
❯ # or
❯ PYTHON_COLORS=0 ./python.exe -m calendar 2020 01 01 2> err
❯ # then
❯ cat err
usage: python.exe -m calendar [-h] [-w WIDTH] [-l LINES] [-s SPACING] [-m MONTHS]
[-c CSS] [-L LOCALE] [-e ENCODING] [-t {text,html}]
[-f FIRST_WEEKDAY]
[year] [month]
python.exe -m calendar: error: unrecognized arguments: 01 |
|
@hugovk Oh thanks, I missed that thread...this is the kind of thing I was afraid of. I'm inclined to say 3.15 onward + a docs update to note this limitation/workaround in 3.14. |
fe5c9d7 to
82e00f0
Compare
|
Circling back on this (appreciate your patience @frostming!), based on this convo, I think we should just move forward with this for 3.15, no backport since this is an API change. For 3.14, we should open a separate PR for the documentation changes for the workaround. I'll leave this open for another couple days to see if anyone else wants to chime in. Otherwise, will merge 😄 ! |
|
@frostming Mind resolving the merge conflicts and then I can get to merging this? 🙂 |
Signed-off-by: Frost Ming <[email protected]>
…olor Signed-off-by: Frost Ming <[email protected]>
Signed-off-by: Frost Ming <[email protected]>
Signed-off-by: Frost Ming <[email protected]>
dc33ac5 to
8518870
Compare
It's done. Thank you very much. |
Fix issue: