GeezerVison

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.

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.

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

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

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? 🙂

@frostming Mind resolving the merge conflicts and then I can get to merging this? 🙂

It's done. Thank you very much.

Perhaps this is the best one can do with the current constraints and large usage of people using internal APIs.

1. It looks like you're mechanically keeping "backwards" compatibility, but you're changing this semantic meaning of the _get_formatter API call
2. Static analyzing tools aren't going to be helpful to debug or update your code to the new semantics
3. If you want to support multiple versions of Python, you'll need to add if sys.version_info >= (3, 14): for both the ArgumentParser(...) constructor as well as the _get_formatter() call.

There's just so much widespread use of internal APIs.

The pattern is this:

formatter = parser._get_formatter()
# ...
parser._print_message(formatter.format_help(), sys.stderr)

https://github.com/dib-lab/khmer/blob/fb65d21eaedf0d397d49ae3debc578897f9d6eb4/khmer/khmer_args.py#L112

https://github.com/ioritzsb/omnipose/blob/4e3a172b41cb34bcefc99b957bf08af21fe39248/omnipose/cli.py#L5

General Search:

https://github.com/search?q=language%3Apython+._get_formatter%28%29&type=code