Skip to content

Exceptions should be reported in a more clear way #420

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

Closed
snejus opened this issue Jul 8, 2020 · 2 comments · Fixed by #421
Closed

Exceptions should be reported in a more clear way #420

snejus opened this issue Jul 8, 2020 · 2 comments · Fixed by #421
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@snejus
Copy link
Contributor

snejus commented Jul 8, 2020

Technical improvement suggestion

Not exactly a bug something that should get some more attention: exception handling and reporting them to the user. I've been working on the configuration reading in main.py module and found that handled exceptions end up being reported like this:

$ mypy -p django-stubs --config-file ./test-data/plugins.ini
Error constructing plugin instance of NewSemanalDjangoPlugin

test-data/plugins.ini:0: error: 'django_settings_module' is not set: no section [mypy.plugins.django-stubs]
Found 1 error in 1 file (checked 477 source files)

image

in comparison to

mypy error with a cmd line argument

$ mypy -p django-stubs --config-file ./tst-data/plugins.ini
usage: mypy [-h] [-v] [-V] [more options; see below]
            [-m MODULE] [-p PACKAGE] [-c PROGRAM_TEXT] [files ...]
mypy: error: Cannot find config file './tst/plugins.ini'

or the unhandled default

You must install the lxml package before you can run mypy with `--html-report`.
You can do this with `python3 -m pip install lxml`.
Traceback (most recent call last):
  File "/home/sarunas/.pyenv/versions/django-stubs-3.6.11/bin/mypy", line 11, in <module>
    sys.exit(console_entry())
  File "/home/sarunas/.pyenv/versions/3.6.11/envs/django-stubs-3.6.11/lib/python3.6/site-packages/mypy/__main__.py", line 8, in console_entry
    main(None, sys.stdout, sys.stderr)
  File "mypy/main.py", line 89, in main
  File "mypy/build.py", line 180, in build
  File "mypy/build.py", line 217, in _build
  File "mypy/report.py", line 58, in __init__
  File "mypy/report.py", line 71, in add_report
ImportError

While the output is the most visually appealing, I find it very quite opaque and misleading. No source files have been checked by that point, so including the count isn't correct; it's also formatted like an issue, so my first reaction (which is basically muscle memory now) is to switch back to the source code and to have a look at the most recent change that has been made. It takes a lot to find that this is an internal problem rather than something that is related to my source files.

I'd think that mypy.Errors may not be the best solution for this - even though it's a very convenient one. There are a couple of unneeded calls made until it reaches a more related CompileError - though it gets thrown plainly, without using its report_internal_error method - which seems like a good option that would include some extras for the user for debugging. Though I guess mypy may have to change into something more related to this project. Though I haven't seen any of them writing to stderr instead of stdout. Also, once the issues are raised, they return None instead of NoReturn - could this indicate that they are not meant to cut off the execution completely?

Though, correct if I'm wrong (I probably am, since I haven't spent days on this :) ), but both of the above seem to be used during the parsing / linting stage. Would it be wrong to assume that reading the configuration should be treated as the setup stage? This way, it could report the error in the same format like mypy does - could also reuse their stuff. This shouldn't be a significant change I reckon.

@snejus snejus added the bug Something isn't working label Jul 8, 2020
@sobolevn
Copy link
Member

sobolevn commented Jul 9, 2020

@snejus I would love to merge a PR with this feature!

@sobolevn sobolevn added the help wanted Extra attention is needed label Jul 9, 2020
@snejus
Copy link
Contributor Author

snejus commented Jul 9, 2020

Perfect! It's in the works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Development

Successfully merging a pull request may close this issue.

2 participants