Better error on empty typeshed directory #3862
Conversation
In addition, mypy now only looks in typeshed/2.7 and no longer looks for 2.6-2.0
mypy/build.py
Outdated
| if sys.platform != 'win32': | ||
| path.append('/usr/local/lib/mypy') | ||
|
|
||
| assert path, "Could not resolve typeshed subdirectories. If you are using MyPy " \ |
There was a problem hiding this comment.
Is there a way to turn this into a regular (blocking) error message? Many people don't read the error when they get a traceback, and just instantly file a bug or go to a help forum.
There was a problem hiding this comment.
I'd like to plead for starting at 2, since mypy still supports 3.2 (as the check target -- not to run mypy itself). It just avoids confusion for people who read the source. (Also it should probably be a DEFINED_CONSTANT somewhere.)
There was a problem hiding this comment.
Im not sure we support 3.2... only 3.3+ are listed in setup.py, and the README states that we support 3.3+. We are planning on dropping 3.3 when it is EOL'd in a month I recall. And I agree it should be a constant.
As to the blocking error message, Im not sure. In general if we are in a state where things are "wrong" eg if there is a key error looking up builtins, that is an exception. I can see how the UX may overrule this. Perhaps raise using report_internal_error in mypy.errors?
There was a problem hiding this comment.
Heh, I forgot about it, but indeed we dropped support for 3.2. (And may it R.I.P. :-)
Regarding the error message, the Error instance hasn't even been constructed at the point this function is called, so the best we could do would be to print(message, file=sys.stderr)and then sys.exit(1). Which isn't terrible, main.py even has a helper that does just that. (Though I think eventually we should refactor the fatal/blocking error handling; I've also got the idea that using sys.exit(2) for all fatal errors would be preferable.)
There was a problem hiding this comment.
we dropped support for 3.2. (And may it R.I.P. :-)
Indeed
RE errors, printing then exiting makes sense. I'll make those changes now.
mypy/build.py
Outdated
| # We allow a module for e.g. version 3.5 to be in 3.4/. The assumption | ||
| # is that a module added with 3.4 will still be present in Python 3.5. | ||
| versions = ["%d.%d" % (pyversion[0], minor) | ||
| for minor in reversed(range(pyversion[1] + 1))] |
There was a problem hiding this comment.
That could probably be range(2, pyversion[1] + 1) since we only support 3.2 and up.
There was a problem hiding this comment.
Typeshed only has 3.3+ so I will do range(3, pyversion[1] + 1).
Instead of using an assert, we check if it is empty, print an error out to sys.stderr, and exit with return 1, so that users will read the error message. Also, we only need to check for folders that exist, thus 3.3 is the minimum.
|
I addressed the two pieces of feedback so this is ready for review again @gvanrossum |
mypy/build.py
Outdated
| # is that a module added with 3.4 will still be present in Python 3.5. | ||
| versions = ["%d.%d" % (pyversion[0], minor) | ||
| for minor in reversed(range(pyversion[1] + 1))] | ||
| for minor in reversed(range(3, pyversion[1] + 1))] |
There was a problem hiding this comment.
It was my understanding that you would replace this magical constant '3' with some globally defined named constant.
There was a problem hiding this comment.
Oh, sorry I must have misunderstood, I thought you wanted that as a separate change. I'll make that change now as part of this.
To avoid magic constants, and make updating minimum version fewer changes, introduce a global constant for the minimum Python 3 version supported.
I see no point in calling out that we don't "really" support 3.0 and 3.1.
|
Thanks! |
|
My pleasure! I'll look through things and see if there is anywhere else that could use the minimum Python version global. |
2 participants
If typeshed is empty (someone forgot to pull the submodule, or the install is broken) the path to typeshed will resolve correctly, but of course stubs will not be found. I added an assert to catch this case. In addition, I realized that before when analyzing 2.7 code, it would look for subdirectories of the typeshed directory for 2.0-2.6, which is pointless, so I modified it to only look in the 2.7 subdirectory.