Skip to content

Add support for nicer error output #7425

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

Merged
merged 6 commits into from
Aug 31, 2019

Conversation

ilevkivskyi
Copy link
Member

Fixes #7368
Fixes #7410

This adds some basic support for output styling on Mac and Linux. The formatter does some basic error message parsing because passing them forward in a structured form would be a bigger change, however if you think this is worth doing I can refactor the PR in a more principled way.

To illustrate the result this is how the output would look:

Screenshot from 2019-08-30 11:22:32

@ilevkivskyi
Copy link
Member Author

I just realized that these changes don't affect the daemon. When we will agree here, I will make a separate PR to update the daemon.

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 30, 2019

Nice! I'd like to play around a little with this before merging to get a feel of how this works.

A few quick comments:

  • The error code looks too prominent to me. Maybe it should be in gray or cyan, for example? It's probably the least interesting thing in the output.
  • It would be interesting to try making the error message bolded/more bright to make it stand out a bit more from the path.
  • Maybe "error" should be in brighter red? Now it's a bit dark.
  • What about using an emoji such as ✨ for success? Something like "✨ All clean" or something. Emojis in error messages seem to be all the rage these days. (No need to do this in this PR, as this could be a bit tricky, depending on the default encoding etc.)

@ilevkivskyi
Copy link
Member Author

@JukkaL OK, I made the changes we discussed privately. Also here is how new output looks on white background.

Screenshot 2019-08-30 16 28 17

@gvanrossum
Copy link
Member

I like the new look on a light theme. (What does it look now on black theme? Still the same?)

I don't recognize Jukka's emoji; let's not go there (error messages are formal speech, where emoji aren't as accepted as in informal speech).

@ilevkivskyi
Copy link
Member Author

What does it look now on black theme? Still the same?

I want to check on my home Linux machine (Mac and Linux may have slightly different colors). I will post a new screenshot when I am back home.

@ilevkivskyi
Copy link
Member Author

Btw I opened mypyc/mypyc#696 for mypyc compilation error.

mypy/util.py Outdated
class FancyFormatter:
"""Apply color and bold font to terminal output.

This currently only works on Linus an Mac.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linux and Mac

mypy/util.py Outdated
if sys.platform not in ('linux', 'darwin'):
self.dummy_term = True
return
if not f_out.isatty() or not f_err.isatty():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need a flag to suppress the isatty checking, since our internal scripts all wrap mypy for logging purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, indeed. I however added just an environment variable, it is probably relatively rarely needed thing.

mypy/util.py Outdated
return out

def underline_link(self, note: str) -> str:
match = re.search(r'http://\S*', note)
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably want https too

@msullivan
Copy link
Collaborator

I love this

@ilevkivskyi
Copy link
Member Author

@gvanrossum I think the new version still works well on dark background:

Screenshot from 2019-08-31 00:35:23

@gvanrossum
Copy link
Member

IMO the contrast for "note" is too low. But presumably it's easy enough to tweak later, so don't worry about it. Or just leave it black?

from typing_extensions import Final, Type, Literal

try:
import curses
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using curses which doesn't support Windows well, perhaps we could use colorama instead? If you don't want to change things in this PR I can open one to redo this in colorama if you want (maybe after the daemon changes).

@ilevkivskyi
Copy link
Member Author

@gvanrossum I actually like it this way. I would keep it and see what people say.

@ethanhs I wanted to start from curses because it is what is in stdlib. But yeah at some point we can switch to something more advanced (if this is required for Windows). Btw, I have heard curses may be available on Windows too but I can't check this. Can you try this PR on Windows (after removing the platform check)?

@gvanrossum
Copy link
Member

No worries. It turns out my love for this is mostly theoretical, since I usually look at mypy errors through various tools (e.g. the PyCharm plugin or Dropbox CI).

IIUC curses on Windows is a 3rd party package: https://pypi.org/project/windows-curses/.

@emmatyping
Copy link
Member

So I tested the PR removing the platform check on Windows:
image

I went ahead and tried the windows-curses package and it returned None for the tigetstr calls, and so it doesn't seem to work, which didn't really surprise me, as I've heard that curses on Windows is rather buggy.

@ilevkivskyi
Copy link
Member Author

I opened some follow-up issues. I would propose to merge this soon (today) and then iterate later (if there are no objections).

@ilevkivskyi ilevkivskyi merged commit 178603c into python:master Aug 31, 2019
@ilevkivskyi ilevkivskyi deleted the add-basic-color branch August 31, 2019 13:43
@ssbarnea
Copy link
Contributor

ssbarnea commented May 7, 2020

What is the trick to enable colors for mypy as I am running on MacOS? All the other python tools are able to use colors but mypy is still plain. I did an import curses and I got no error, so I doubt is that. Funny, all the CLI options seem to be about how to disable it not how to enforce it.

I tried: mypy --color-output lib and clearly it lacks any colors.

@emmatyping
Copy link
Member

@ssbarnea please open a new issue describing what terminal you are using and any other relevant information.

@ssbarnea
Copy link
Contributor

ssbarnea commented May 8, 2020

@ethanhs Thanks, added as #8791

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

Successfully merging this pull request may close these issues.

Use colors in mypy output Make mypy more positive
6 participants