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

Improve presentation of diagnostic errors using rich #10703

Merged
merged 10 commits into from
Dec 14, 2021

Conversation

pradyunsg
Copy link
Member

Toward #10421

Before:

Screenshot 2021-12-03 at 14 24 29

After:

Screenshot 2021-12-03 at 14 23 53

@pradyunsg pradyunsg force-pushed the rich-diagnostic-error-messages branch 2 times, most recently from 2624afb to e144173 Compare December 3, 2021 14:47
@pradyunsg pradyunsg added C: error messages Improving error messages C: output Related to what pip prints type: enhancement Improvements to functionality labels Dec 3, 2021
@pradyunsg pradyunsg force-pushed the rich-diagnostic-error-messages branch from 2baace7 to bf6b732 Compare December 3, 2021 15:09
@pradyunsg
Copy link
Member Author

Ooops, I thought I'd opened this as a draft. Well, assuming the CI passes, this is ready for review! :)

@DiddiLeija
Copy link
Member

Just update tests/functional/test_no_color.py, because it failed:

>       assert "\x1b" in get_run_output(option=""), "Expected color in output"
E       AssertionError: Expected color in output
E       assert '\x1b' in 'Script started on 2021-12-03 15:22:51+00:00 [<not executed on terminal>]\nWARNING: Skipping noSuchPackage as it is not installed.\n\nScript done on 2021-12-03 15:22:51+00:00 [COMMAND_EXIT_CODE="0"]\n'
E        +  where 'Script started on 2021-12-03 15:22:51+00:00 [<not executed on terminal>]\nWARNING: Skipping noSuchPackage as it is not installed.\n\nScript done on 2021-12-03 15:22:51+00:00 [COMMAND_EXIT_CODE="0"]\n' = <function test_no_color.<locals>.get_run_output at 0x7f8774180040>(option='')

Seems like the behavior has changed here.

But from the linting side, everything looks good 👍

@pradyunsg
Copy link
Member Author

Yea, that test is going to be non-trivial to update.

The failure is basically because rich is smarter than pip's old logic about whether colours would make sense, and this breaks the assumption that the test was written with. I'm leaning toward redoing that test, to be pty-based to get colours working again, but I'd like to see if anyone else has opinions/thoughts on this. :)

@uranusjr uranusjr marked this pull request as draft December 4, 2021 07:45
@pradyunsg pradyunsg marked this pull request as ready for review December 4, 2021 19:05
@pradyunsg
Copy link
Member Author

Hurray! Ready for review now that I figured out how to make the test work on MacOS and Ubuntu. :)

@pradyunsg
Copy link
Member Author

pradyunsg commented Dec 4, 2021

The failure is basically because rich is smarter than pip's old logic about whether colours would make sense, and this breaks the assumption that the test was written with. I'm leaning toward redoing that test, to be pty-based to get colours working again, but I'd like to see if anyone else has opinions/thoughts on this. :)

This was wrong and I'm glad that the test broke.

I'd changed ERROR and WARNING log messages to no longer be red / yellow. Not sure if anyone feels strongly about this, but hey, that's what code review is supposed to be for -- and the test did the right thing. :)

yield f"{self.message}\n"
if self.context is not None:
yield f"\n{self.context}\n"
self.link = link
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hope is that, in the future, these can be generated using the reference and somehow be synchronised with the documentation... but let's keep this as is for now. :)

@pradyunsg pradyunsg force-pushed the rich-diagnostic-error-messages branch 2 times, most recently from 3f4a835 to cd8edc8 Compare December 4, 2021 19:19
@uranusjr
Copy link
Member

uranusjr commented Dec 4, 2021

I've changed ERROR and WARNING log messages to no longer be red / yellow.

What are they now? (I didn’t read the change yet.)

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Dec 4, 2021
@pradyunsg pradyunsg force-pushed the rich-diagnostic-error-messages branch from cd8edc8 to c429c82 Compare December 5, 2021 10:55
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Dec 5, 2021
@pradyunsg
Copy link
Member Author

What are they now? (I didn’t read the change yet.)

They were unstylized basically. I realised that this might not be ideal while we're transitioning over though, so I've gone ahead and readded the colours for this.

@pradyunsg pradyunsg force-pushed the rich-diagnostic-error-messages branch from c429c82 to 96b0ab1 Compare December 5, 2021 11:16
@pradyunsg pradyunsg marked this pull request as draft December 5, 2021 12:01
@pradyunsg pradyunsg force-pushed the rich-diagnostic-error-messages branch from d653e65 to 2009a22 Compare December 5, 2021 12:52
@pradyunsg pradyunsg marked this pull request as ready for review December 5, 2021 12:52
@pradyunsg
Copy link
Member Author

Rebased, and should be good to review now!

@hugovk
Copy link
Contributor

hugovk commented Dec 5, 2021

Good work, so much more readable than a wall of red! 🎨

@ashb
Copy link

ashb commented Dec 5, 2021

Yea, that test is going to be non-trivial to update.

The failure is basically because rich is smarter than pip's old logic about whether colours would make sense, and this breaks the assumption that the test was written with. I'm leaning toward redoing that test, to be pty-based to get colours working again, but I'd like to see if anyone else has opinions/thoughts on this. :)

Isn't using a pty in the test effectively re-testing Rich's own tested code?

@pradyunsg
Copy link
Member Author

Isn't using a pty in the test effectively re-testing Rich's own tested code?

No. It's testing that the output respects pip's own --no-color flag.

Copy link

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f strings with console markup is a bit of a gotcha with Rich. See escaping for details...

src/pip/_internal/exceptions.py Show resolved Hide resolved
src/pip/_internal/exceptions.py Outdated Show resolved Hide resolved
renderable = self.render_message(record, message)
if record.levelno is not None:
if record.levelno >= logging.ERROR:
style = Style(color="red")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to consider using Rich themes. That way you can define the styles in one place.

Copy link
Member Author

@pradyunsg pradyunsg Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I will probably do something like that in the future. For now, I want to keep this as a 1:1 replacement to avoid making/adding another indirection to think of while reviewing this.

@pradyunsg
Copy link
Member Author

Nudging @pypa/pip-committers for a review on this. :)

@pradyunsg
Copy link
Member Author

Oh, and this is easiest to review commit-by-commit. The first commit is what makes the patch big.

@pradyunsg
Copy link
Member Author

This doesn't really have any functional behaviour changes (i.e. it won't change what gets installed or stuff like that), so I am somewhat comfortable just merging this without having it go through a rigorous review from another maintainer.

I'd still strongly prefer to get some more 👀 through this PR, in a manner similar to other PRs for #10421.

@hugovk
Copy link
Contributor

hugovk commented Dec 12, 2021

Testing this branch (macOS Monterey, Python 3.10.1, iTerm2), I get slightly different output to the example up top:

image

It also highlights the digits of the PEP number here. Not a big deal, I note this is how vanilla rich works:

image

Borrow error presentation logic from sphinx-theme-builder, and
exhaustively test both the unicode and non-unicode presentation.

Utilise rich for colours and presentation logic handling, with tests to
ensure that colour degradation happens cleanly, and that the content is
stylized exactly as expected.

Catch diagnostic errors eagerly, and present them using rich. While this
won't include the pretty presentation in user logs, those files will
contain the entire traceback upto that line.
This makes it possible to present output with rich markup, within the
constraints of our logging infrastructure.

Further, diagnostic errors can now by presented using rich, using their
own special "[present-diagnostic]" marker string, since those need to be
handled differently from regular log messages and passed directly
through to rich's console object, after an indentation wrapper.
The new name is a closer match with what is presented to the user.
This is only necessary on messages which may contain `[`, which needs to
be escaped before printing with rich, since rich treats them as output
formats.
This is necessary to ensure that the output can be compared directly.
Also, reflects that `--no-color` does not mean no-ansi-escapes but only
affects colour rules.
This is a slightly cleaner presentation style.
@pradyunsg pradyunsg force-pushed the rich-diagnostic-error-messages branch from 7a0f28d to 6b31f83 Compare December 12, 2021 13:13
@pradyunsg
Copy link
Member Author

Indeed! Fixed both of those issues, which were both caused by mishandling in 7d4d84e.

Screenshot 2021-12-12 at 13 13 33

Copy link
Contributor

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor suggestions.

news/10703.feature.rst Outdated Show resolved Hide resolved
src/pip/_internal/utils/logging.py Outdated Show resolved Hide resolved
src/pip/_internal/exceptions.py Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@pradyunsg
Copy link
Member Author

pradyunsg commented Dec 14, 2021

Going to go ahead and merge this now. If there's any concerns with this, please feel free to holler over at #10421! :)

@pradyunsg pradyunsg merged commit 5cf9840 into pypa:main Dec 14, 2021
@pradyunsg pradyunsg deleted the rich-diagnostic-error-messages branch December 14, 2021 19:13
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: error messages Improving error messages C: output Related to what pip prints type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants