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

Allow setting python_executable from config file... #5777

Merged
merged 8 commits into from
Dec 17, 2018

Conversation

ethanhs
Copy link
Collaborator

@ethanhs ethanhs commented Oct 12, 2018

... and let the python_version and python_executable be different, so that one can install stub packages into the Python used to run mypy.

Fixes #5620.

@ethanhs ethanhs changed the title Allow setting python_version/executable from config file... [WIP] Allow setting python_version/executable from config file... Oct 12, 2018
@ethanhs
Copy link
Collaborator Author

ethanhs commented Oct 12, 2018

I realized this isn't actually correct yet, so I'm not quite ready for review yet.

@ethanhs ethanhs changed the title [WIP] Allow setting python_version/executable from config file... Allow setting python_version/executable from config file... Oct 24, 2018
@ethanhs
Copy link
Collaborator Author

ethanhs commented Oct 24, 2018

It appears the new version of flake8 is very picky. The Travis failure is not related to these changes (its just flake8). I will submit a PR to fix this.

@ethanhs
Copy link
Collaborator Author

ethanhs commented Oct 24, 2018

Okay this should be ready for review.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

This looks good.

It might be worth adding a test that actually does read the version from a config file

@ethanhs
Copy link
Collaborator Author

ethanhs commented Oct 25, 2018

It might be worth adding a test that actually does read the version from a config file

There are several tests for reading the version from the config file already (in cmdline.test). I would do the same for python_executable, but that is a bit harder to test. I suppose I could add something to testpep561 if such a test is desired.

@ethanhs
Copy link
Collaborator Author

ethanhs commented Oct 31, 2018

I will merge this later today unless someone has suggestions for changes. (@msullivan not sure if you wanted to respond to my above comment).

mypy/main.py Outdated Show resolved Hide resolved
@ethanhs ethanhs changed the title Allow setting python_version/executable from config file... Allow setting python_executable from config file... Nov 1, 2018
@gvanrossum
Copy link
Member

gvanrossum commented Nov 13, 2018

I am trying to come up with an end-to-end test for this, and it's not working.

I found a pypi package ordered_set (aka ordered-set) that has stubs on pypi.

I am installing these with:

$ python2.7 -m pip install ordered_set ordered_set_stubs

To prove that it works:

$ python2.7 -c 'import ordered_set'

This gives no errors. Then with mypy from master, I run:

$ mypy -2 -c 'import ordered_set'
/Users/guido/v27/lib/python2.7/site-packages/ordered_set-stubs/__init__.pyi:10: error: Cannot determine consistent method resolution order (MRO) for "OrderedSet"

This error is expected (there's a bug in the stubs that only happens to manifest with Python 2). (Note that ~/v27 is the venv I use.)

But with mypy from this branch I can't get it to work:

$ mypy -2 -c 'import ordered_set'
<string>:1: error: Cannot find module named 'ordered_set'
<string>:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)

I can get it to work by installing the stubs in the Python 3 venv I use to run mypy, or by explicitly specifying a Python executable:

$ mypy -2 -c 'import ordered_set' --python-executable ~/v27/bin/python
/Users/guido/v27/lib/python2.7/site-packages/ordered_set-stubs/__init__.pyi:10: error: Cannot determine consistent method resolution order (MRO) for "OrderedSet"

But that seems a step back.

assert options.python_version == sys.version_info[:2]
assert options.python_executable == sys.executable

# then test inferring version from executable
Copy link
Member

Choose a reason for hiding this comment

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

Is this testing anything any more? You don't infer the version from the executable any more in the code.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

(Unapproving since I can't get it to work as I expect end-to-end.)

@ethanhs
Copy link
Collaborator Author

ethanhs commented Nov 13, 2018

Ah, yes I have it using the default Python executable. I will need to think this over a bit. I believe the issue is that we want to be able to use options.python_executable, but it already has a default, so there is no way to tell if the config file has set the value and we don't need to do inference, or if it just is using the default value and we want to infer the correct executable based on the Python version.

I suppose the "correct" thing to do would probably be to make python_executable and python_version default to None when unset and have the entry point set their defaults if all else fails.

@gvanrossum
Copy link
Member

Hm, it looks like the logic for the Python version could be simplified: just use the default dest (don't bother with special-opts:python_version). For executable I think you could also do away with special-opts: in options.py you set the default to None and then in main.py you compute the value dynamically like this:

  • if explicitly specified, keep that
  • else if options.no_executable is set, use None
  • else compute from version

It looks like everything becomes easier that way.

@gvanrossum
Copy link
Member

@ethanhs Any change to get this in for the 0.650 release? I think it would be a nice improvement.

@gvanrossum
Copy link
Member

(If you want to push back on my suggestion to simplify the implementation and just merge the current version I could be okay with that too.)

@gvanrossum gvanrossum dismissed their stale review December 17, 2018 19:50

Decided I want the fix more badly than the simplified implementation.

@gvanrossum gvanrossum merged commit d2a9f9d into python:master Dec 17, 2018
@ethanhs
Copy link
Collaborator Author

ethanhs commented Dec 17, 2018

Oh, sorry I seem to have missed your message from two weeks back. I will make another PR tomorrow with the simplification.

@ethanhs ethanhs deleted the relaxpyexe branch December 17, 2018 21:16
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.

3 participants