gh-139946: distinguish stdout or stderr when colorizing output in argparse

[frostming]

Fix issue:

• Errors raised in argparse are colorized when redirecting stderr to a file #139946

[bedevere-app]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[savannahostrowski]

Thanks for addressing the comments. After thinking about this more, I'm a bit concerned about API compatibility. Adding the formatter parameter to format_usage() and format_help() changes the public API. While it has a default value, this could still break subclasses that override these methods. I would like to backport a fix for this bug, but maybe @hugovk has thoughts on this change as RM.

Also, we'll need to add some tests here, since this wasn't caught with our existing test suite.

[hugovk]

I would like to backport a fix for this bug, but maybe @hugovk has thoughts on this change as RM.

After originally adding colour to argparse (#132323), we did get reports (#133653) that subclassing HelpFormatter was broken:

TypeError: HelpFormatter.__init__() got an unexpected keyword argument 'prefix_chars'

We initially considered this a docs issue, because HelpFormatter isn't fully documented (#133653 (comment)) but then fixed it in another way (#133941).

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 or PYTHON_COLORS=0 when redirecting output:

❯ 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

[savannahostrowski]

@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.

[gpshead]

this is fine for 3.15 but is a public API change so we shouldn't backport this as a bugfix as is.

what I'd do is change these to _format_usage and _format_help functions with the additional formatter= parameter, and add backwards compatibility public format_usage and format_help methods without any parameters (existing public API) that call those.

[gpshead]

even that might not be enough though given hugo's comment about subclass overrides though as people probably expect those methods to be called by our other internals.

another backportable workaround: setting an instance variable with a formatter or file... getting gross though.

[frostming]

even that might not be enough though given hugo's comment about subclass overrides though as people probably expect those methods to be called by our other internals.

That wouldn't help, the only place they are called are in the print_usage() and print_help() methods. If we change the method name and internal calls the subclasses won't work either. We only make two public format_* methods never be called.

A usable solution is to inspect the method’s signature, but that would be a bit strange.

[gpshead]

rather than invoking inspect (heavyweight) I'd write it as: try the call with the kwarg and if that raises a TypeError, fallback to the call without the kwarg. that should handle a subclass not having the modern API.

[gpshead]

I don't like adding the argument to the API in a bugfix backport though. These format_ methods are documented as something for subclasses to override in the argparse docs suggesting even in a stable release changing their required signature could make adoption painful.

Is it really a problem if we temporarily set a new _instance_attribute for each one that the fixed versions consume? That'd avoid changing their signature.

[frostming]

Is it really a problem if we temporarily set a new _instance_attribute for each one that the fixed versions consume? That'd avoid changing their signature.

The problem is print_usage() doesn't pass file argument to the formatter created by format_usage, so from the perspective of formatter, it doesn't know which stream it should format against.

So if we don't pass additional info to format_ methods, we would have to set some state in parser itself because the formatter isn't created beforehand.

[mpkocher]

I don't like adding the argument to the API in a bugfix backport though. These format_ methods are documented as something for subclasses to override in the argparse docs suggesting even in a stable release changing their required signature could make adoption painful.

Some context:

1. The lack of progress on trying improve/formalize the public API HelpFormatter (from 2017) and Argparse issues (view)) has created some fundamental issues.
2. People have resorted to using private methods to solve these frictions points in the API design (example). The line between public and private is quite blurry and now there's a "nothing can really change" state that has been calcified.
3. Every subclass of Action has access to the world via the parser instance in __call__. This often requires reaching into private methods of parser (e.g.,, ._get_formatter() amongst other things. VersionAction is a simple example, or another example