Use red text when printing errors on command line

[callmecampos]

Per discussion in #511, when twine emits error messages on the command line, we now print them in red.

Here are a couple examples from my terminal on macOS Catalina (v10.15.4):

Before merging, I want to address some of the following:

• Gracefully handle colorama not being available (only necessary in pip/logging.py since they've vendored colorama)
• (potentially) Include a flag for not coloring any twine output and consider a "holistic approach to styling twine's output" (as in pip/logging.py)

And an example with the --no-color flag:

[di]

Gracefully handle colorama not being available

Seems like doing something to what pip does makes sense here:

try:
    import colorama
# Lots of different errors can come from this, including SystemError and
# ImportError.
except Exception:
    colorama = None

if colorama:
    ...

It would be nice to actually confirm these changes would be handled gracefully on Windows, I created #650 to explore testing against these platforms in CI.

(potentially) Include a flag for not coloring any twine output

I'd take a look at pypa/pip#8135 and see if we want to mimic the same flag here.

[bhrutledge]

Thanks for adding this! I haven't tried it yet, but I'm looking forward to it. I've got some questions/suggestions before merging.

For extra credit, could you include an example screenshot?

[bhrutledge]

Given the open questions and TODOs, I've converted this to a draft PR.

[callmecampos]

@bhrutledge per your request, I included some example screenshots above of what it looks like

[bhrutledge]

Nice improvements. I think there's still some work to do re: the --no-color option.

Also, thanks for graciously accepting my earlier marking of the PR as a draft, and for marking it as "Ready For Review". In retrospect, I didn't need to do that, and can see how it might have been confusing/surprising.

[bhrutledge]

Thanks, @callmecampos. Unfortunately, the Settings approach here won't work. Feel free to ask questions.

[bhrutledge]

The Settings class is currently only for upload and register settings, so this approach won't work, because it adds all of those options to check:

$ twine check --help
usage: twine check [-h] [-r REPOSITORY] [--repository-url REPOSITORY_URL] [-s]
                   [--sign-with SIGN_WITH] [-i IDENTITY] [-u USERNAME]
                   [-p PASSWORD] [--non-interactive] [-c COMMENT]
                   [--config-file CONFIG_FILE] [--skip-existing] [--cert path]
                   [--client-cert path] [--verbose] [--disable-progress-bar]
                   [--no-color]
                   dist [dist ...]

Compared to the current:

$ twine check --help
usage: twine check [-h] dist [dist ...]

[di]

@bhrutledge How do you propose we share commands like --no-color between multiple sub-commands instead? In #659, @sigmavirus24 said that the Settings object should be used for this.

[sigmavirus24]

So far we've needed thse arguments for all the commands and check was implemented while I was not paying attention. Otherwise, we'd have resolved this sooner. I think separate classmethods for things that don't want need all of those arguments is the way to go. Then register_argparse_arguments can call those and then add its own arguments.

[di]

I agree that check sort of skirted this issue and should eventually be fixed.

I think separate classmethods for things that don't want need all of those arguments is the way to go.

Can you elaborate on this? This kind of sounds like what #659 could solve with class composition/inheritance. Do you mean extra classmethods on the Settings class instead? Something like Settings._add_register_and_upload_arguments?

[sigmavirus24]

Can you elaborate on this?

Sure.

This kind of sounds like what #659 could solve with class composition/inheritance.

Except that the TwineCommand class would never be part of the public API the way Settings would be as an escape hatch from the high-level porcelain API planned.

I'm thinking of breaking up registration into class methods based on relationship:

Settings.register_repository_arguments(...)
Settings.register_gpg_arguments(...)
Settings.register_identity_arguments(...)

And those are called by

class Settings:
    @classmethod
    def register_upload_arguments(cls, ...):
        cls.register_repository_arguments(...)
        cls.register_gpg_arguments(...)
        cls.register_identity_arguments(...)
        # plus anything specific to upload

Likewise for check and register. I doubt register should have --comment or --skip-existing, so those would be specific to upload. This would give us logical groupings that are more explicit than what we have no and allow commands to register what they thematically need.

[di]

@sigmavirus24 would you like @callmecampos to move forward with that in this PR? Should it come before this PR?

[sigmavirus24]

I would be worried about that blocking this PR. As a transitory state, we could have check use all the options and in @callmecampos's next PR they could "fix" it.

[bhrutledge]

Re: adding class methods to Settings, I support that in theory, but not as part of this PR. I also note that there are number of pending issues to improve check, which makes me wonder if the Settings restructuring could be part of that work, and maybe released as Twine 4.0, to give us some flexibility on the API.

[sigmavirus24]

and maybe released as Twine 4.0, to give us some flexibility on the API.

Why as 4.0? Also perhaps this isn't the best place to have this long-winded discussion

[sigmavirus24]

I'm also worried about how we're approaching this. We don't seem focused or aligned on a goal right now and instead we're bandying about solutions and focusing on those.

What are our goals and constraints for the solution beyond "Can disable color?"

[bhrutledge]

Thanks for accepting my proposal. :)

[bhrutledge]

@callmecampos This was released in 3.2.0. Thanks again for your work on this.