Skip to content
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

Add color to warnings #818

Closed
bhrutledge opened this issue Oct 7, 2021 · 12 comments
Closed

Add color to warnings #818

bhrutledge opened this issue Oct 7, 2021 · 12 comments
Assignees

Comments

@bhrutledge
Copy link
Contributor

bhrutledge commented Oct 7, 2021

While working on #815 and #817, I remembered a suggestion for this. I think it would make them more obvious.

pip does this, with a custom logging.StreamHandler.

There are also a couple packages that hook into logging and use colorama (which Twine already uses for the red errors):

I think part of implementing this should be normalizing the way Twine reports warnings; it currently uses logger.warning and warnings.warn. It seems like the former is preferable for this purpose, although logging.captureWarnings could be a fallback.

Similarly, we might want to rework the red error implementation from #649 to use logger.error.

@bhrutledge
Copy link
Contributor Author

I'm keen to work on this, but would appreciate ideas/suggestions before proceeding.

@sigmavirus24
Copy link
Member

Why wouldn't we use something like rich? It works fantastically and would allow us to improve our other output over the long run with its significantly better progress bars and cross-OS support.

@bhrutledge
Copy link
Contributor Author

Why wouldn't we use something like rich?

Funny you should ask; yesterday I was listening to a podcast interview with the maintainer, and was thinking "could Twine unify all of its output (including progress bars) with Rich?"

I've been looking for an excuse to play with that library, and would be happy to do that for Twine.

@sigmavirus24 Do you have suggestions on a roadmap for migrating to Rich?

@sigmavirus24
Copy link
Member

Do you have suggestions on a roadmap for migrating to Rich?

Can you share more about what you're thinking about here?

It sounds like you want a kind of phased transition to it and I'm curious if that's right and what is leading you to that desire

@bhrutledge
Copy link
Contributor Author

bhrutledge commented Oct 7, 2021

I think my overall desire is to unify Twine's output under a single, consistent API. Off the top of my head, and having not thoroughly investigated Rich's features, that could include:

@sigmavirus24
Copy link
Member

Ah, I thought you were asking about introducing rich behind some sort of feature-flag to gather feedback from folks as opposed to making a hard switch.

  • Migrate logger.warning and warnings.warn to Rich

So, I'll repeat that the easiest way to implement --verbose is via the logging module which allows us to map --verbose to a setting on the logger root. Otherwise code will have to have if options.verbose > arbitrary_number_constant: print(...) which can be unified in a single place, but also adds maintenance cost. Python's stdlib logging allows you to define your own levels as well, so it's not as if it will limit our ability to provide fine-grained verbose output if/when we allow --verbose to be repeatable.

As for warnings.warn I genuinely have no clue why we ever used that. That is not appropriate for a CLI like ours. It was a mistake and we should definitely get rid of that ASAP.

Finally, Rich has it's own built-in logging handler: https://rich.readthedocs.io/en/stable/logging.html which is what made me think of this in the first place.

  • Migrate Colorama error text to Rich
  • Migrate print() to Rich
  • Migrate tqdm progress bar to Rich
  • Use Rich for pretty tracebacks

👍 to all of these.

@bhrutledge
Copy link
Contributor Author

the easiest way to implement --verbose is via the logging module which allows us to map --verbose to a setting on the logger root.

I think we're already doing this:

twine/twine/settings.py

Lines 149 to 155 in 4c9d8c1

@verbose.setter
def verbose(self, verbose: bool) -> None:
"""Initialize a logger based on the --verbose option."""
self._verbose = verbose
root_logger = logging.getLogger("twine")
root_logger.addHandler(logging.StreamHandler(sys.stdout))
root_logger.setLevel(logging.INFO if verbose else logging.WARNING)

As for warnings.warn I genuinely have no clue why we ever used that.

Okay. I'll aim to normalize to logger.warning and color that with Rich.

@bhrutledge
Copy link
Contributor Author

In my proposed roadmap in #818 (comment), I've added an entry for migrating check, because it writes to stdout in a way that doesn't seem amenable to logging:

def check(
dists: List[str],
output_stream: IO[str] = sys.stdout,
strict: bool = False,
) -> bool:

This reminds me of the discussion about refactoring commands in #659, but I'm not inclined to go down that rabbit hole right now.

@sigmavirus24
Copy link
Member

To be clear, there's no agreement in #659 and no real problem statement. It seems to be in service of using commands to provide a public API which I'm emphatically against.

I wouldn't think of #659 as committed work until we can agree what the actual problem is

@bhrutledge bhrutledge changed the title Use yellow text for warnings Add color to warnings Jan 2, 2022
@bhrutledge bhrutledge self-assigned this Jan 25, 2022
@bhrutledge
Copy link
Contributor Author

@sigmavirus24 @di My next step is to migrate check's warnings and errors to Rich. However, check doesn't use logging; instead, it takes an output_stream argument:

def check(
dists: List[str],
output_stream: IO[str] = sys.stdout,
strict: bool = False,
) -> bool:

It seems that's only used for testing, e.g.:

twine/tests/test_check.py

Lines 99 to 101 in 12469ce

output_stream = io.StringIO()
assert not check.check(["dist/*"], output_stream=output_stream)
assert output_stream.getvalue() == "Checking dist/dist.tar.gz: PASSED\n"

My preference would be to remove output_stream and use logging, plus pytest fixtures like capsys and caplog to test the output. However, that would be an API change. Do you think that's acceptable, if we release it as 4.0?

Alternatively, I'm guessing it's possible to configure logging to use output_stream. Or, check could use Rich directly, instead of logging.

@sigmavirus24
Copy link
Member

My preference would be to remove output_stream and use logging, plus pytest fixtures like capsys and caplog to test the output. However, that would be an API change. Do you think that's acceptable, if we release it as 4.0?

Yes, let's do that. Also, I'll reiterate, twine.commands shouldn't be considered a public API and we should be free to break it outside of a major release as well.

@bhrutledge
Copy link
Contributor Author

I've updated the checklist in #818 (comment), choosing to omit tracebacks because they seem too rich. So, I think this is done, and further enhancements can be suggested in new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants