Improve presentation of diagnostic errors using rich #10703

pradyunsg · 2021-12-03T14:32:09Z

Before:

After:

pradyunsg · 2021-12-03T15:17:00Z

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

DiddiLeija · 2021-12-03T20:33:15Z

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 · 2021-12-03T21:03:57Z

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. :)

pradyunsg · 2021-12-04T19:05:37Z

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

pradyunsg · 2021-12-04T19:07:21Z

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. :)

pradyunsg · 2021-12-04T19:09:57Z

src/pip/_internal/exceptions.py

-        yield f"{self.message}\n"
-        if self.context is not None:
-            yield f"\n{self.context}\n"
+        self.link = link


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. :)

uranusjr · 2021-12-04T20:31:59Z

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

pradyunsg · 2021-12-05T10:57:15Z

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 · 2021-12-05T12:58:57Z

Rebased, and should be good to review now!

hugovk · 2021-12-05T13:03:30Z

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

ashb · 2021-12-05T13:09:47Z

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 · 2021-12-05T13:36:51Z

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.

willmcgugan

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

src/pip/_internal/exceptions.py

willmcgugan · 2021-12-06T10:10:44Z

src/pip/_internal/utils/logging.py

+            renderable = self.render_message(record, message)
+            if record.levelno is not None:
+                if record.levelno >= logging.ERROR:
+                    style = Style(color="red")


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

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 · 2021-12-10T00:29:46Z

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

pradyunsg · 2021-12-10T00:31:53Z

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

pradyunsg · 2021-12-12T12:27:43Z

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 · 2021-12-12T13:01:07Z

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

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

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 · 2021-12-12T13:14:49Z

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

hugovk

Some minor suggestions.

news/10703.feature.rst

src/pip/_internal/utils/logging.py

src/pip/_internal/exceptions.py

Co-authored-by: Hugo van Kemenade <[email protected]>

pradyunsg · 2021-12-14T19:13:54Z

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

pradyunsg force-pushed the rich-diagnostic-error-messages branch 2 times, most recently from 2624afb to e144173 Compare December 3, 2021 14:47

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 force-pushed the rich-diagnostic-error-messages branch from 2baace7 to bf6b732 Compare December 3, 2021 15:09

uranusjr marked this pull request as draft December 4, 2021 07:45

pradyunsg marked this pull request as ready for review December 4, 2021 19:05

pradyunsg commented Dec 4, 2021

View reviewed changes

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

github-actions bot added the needs rebase or merge PR has conflicts with current master label Dec 4, 2021

pradyunsg force-pushed the rich-diagnostic-error-messages branch from cd8edc8 to c429c82 Compare December 5, 2021 10:55

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Dec 5, 2021

pradyunsg force-pushed the rich-diagnostic-error-messages branch from c429c82 to 96b0ab1 Compare December 5, 2021 11:16

pradyunsg marked this pull request as draft December 5, 2021 12:01

pradyunsg force-pushed the rich-diagnostic-error-messages branch from d653e65 to 2009a22 Compare December 5, 2021 12:52

pradyunsg marked this pull request as ready for review December 5, 2021 12:52

pradyunsg mentioned this pull request Dec 5, 2021

Add additional diagnostics to our error messages #10421

Open

willmcgugan reviewed Dec 6, 2021

View reviewed changes

This was referenced Dec 10, 2021

Fallback to pep517 if setup.py is present and setuptools cannot be imported #10717

Merged

Improve our setuptools shim #10723

Closed

pradyunsg added this to the 22.0 milestone Dec 12, 2021

pradyunsg added 9 commits December 12, 2021 13:11

Replace {attention -> note}_stmt

9f4117f

The new name is a closer match with what is presented to the user.

Clean up where Text is used to wrap diagnostic messages.

d517dae

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.

Add news fragment

6a20f5b

Properly handle newlines in diagnostic error tests

e857d0f

This is necessary to ensure that the output can be compared directly.

Restore colors in regular logging messages

b82dfd1

Also, reflects that `--no-color` does not mean no-ansi-escapes but only affects colour rules.

Stop stylizing PEP **518**

961eb6f

This is a slightly cleaner presentation style.

Escape the package name, allowing rich to stylize URLs and links

6b31f83

pradyunsg force-pushed the rich-diagnostic-error-messages branch from 7a0f28d to 6b31f83 Compare December 12, 2021 13:13

hugovk reviewed Dec 12, 2021

View reviewed changes

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

pradyunsg mentioned this pull request Dec 12, 2021

Embrace | for union of types #10725

Open

Apply suggestions from code review

fd45825

Co-authored-by: Hugo van Kemenade <[email protected]>

pradyunsg merged commit 5cf9840 into pypa:main Dec 14, 2021

pradyunsg deleted the rich-diagnostic-error-messages branch December 14, 2021 19:13

webknjaz mentioned this pull request Dec 21, 2021

ansible-galaxy - validate version for directories and collections in git repos ansible/ansible#76579

Merged

github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve presentation of diagnostic errors using rich #10703

Improve presentation of diagnostic errors using rich #10703

pradyunsg commented Dec 3, 2021

pradyunsg commented Dec 3, 2021

DiddiLeija commented Dec 3, 2021

pradyunsg commented Dec 3, 2021

pradyunsg commented Dec 4, 2021

pradyunsg commented Dec 4, 2021 •

edited

Loading

pradyunsg Dec 4, 2021

uranusjr commented Dec 4, 2021

pradyunsg commented Dec 5, 2021

pradyunsg commented Dec 5, 2021

hugovk commented Dec 5, 2021

ashb commented Dec 5, 2021

pradyunsg commented Dec 5, 2021

willmcgugan left a comment

willmcgugan Dec 6, 2021

pradyunsg Dec 6, 2021 •

edited

Loading

pradyunsg commented Dec 10, 2021

pradyunsg commented Dec 10, 2021

pradyunsg commented Dec 12, 2021

hugovk commented Dec 12, 2021

pradyunsg commented Dec 12, 2021

hugovk left a comment

pradyunsg commented Dec 14, 2021 •

edited

Loading

Improve presentation of diagnostic errors using rich #10703

Improve presentation of diagnostic errors using rich #10703

Conversation

pradyunsg commented Dec 3, 2021

pradyunsg commented Dec 3, 2021

DiddiLeija commented Dec 3, 2021

pradyunsg commented Dec 3, 2021

pradyunsg commented Dec 4, 2021

pradyunsg commented Dec 4, 2021 • edited Loading

pradyunsg Dec 4, 2021

Choose a reason for hiding this comment

uranusjr commented Dec 4, 2021

pradyunsg commented Dec 5, 2021

pradyunsg commented Dec 5, 2021

hugovk commented Dec 5, 2021

ashb commented Dec 5, 2021

pradyunsg commented Dec 5, 2021

willmcgugan left a comment

Choose a reason for hiding this comment

willmcgugan Dec 6, 2021

Choose a reason for hiding this comment

pradyunsg Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

pradyunsg commented Dec 10, 2021

pradyunsg commented Dec 10, 2021

pradyunsg commented Dec 12, 2021

hugovk commented Dec 12, 2021

pradyunsg commented Dec 12, 2021

hugovk left a comment

Choose a reason for hiding this comment

pradyunsg commented Dec 14, 2021 • edited Loading

pradyunsg commented Dec 4, 2021 •

edited

Loading

pradyunsg Dec 6, 2021 •

edited

Loading

pradyunsg commented Dec 14, 2021 •

edited

Loading