Update -2 argument dest to match --python-version.

[sqwishy]

And check config for python_version when looking for a python
executable.

---

See issue #5576.

I didn't add any automated tests because I wasn't sure what you guys would have wanted for that. This stuff makes use of subprocess so either that'd be mocked out or it would depend on python2.7 being installed? I'm not sure.

And I'm uncertain that changing infer_python_version_and_executable() as I did is a good idea. Or if you'd prefer that python_executable on the special_opts namespace be set, if it's None, to the value of that attribute on the Options instance somewhere in process_options() before calling infer_python_version_and_executable()?

[ethanhs]

I will take a closer look later today on the contents. As for testing, I think the best place to add it is here: https://github.com/python/mypy/blob/master/mypy/test/testargs.py#L26.

[sqwishy]

@ethanhs Yeah I had looked at that earlier, for some reason I thought I'd need to test where options.python_version = (2, 7) and it would then depend on python 2.7 being installed. But I'm pretty sure I can use sys.version_info like the rest of that test. So I'll try that.

And if you decide that infer_python_version_and_executable() probably shouldn't check both objects for the python_version (that the caller should make up its mind and put those objects in a more agreeable state), then we can come up with a different test.

[ethanhs]

@sqwishy ah, you probably want to use https://github.com/python/mypy/blob/master/mypy/util.py#L101 which will return None if it cannot find a Python 2 interpreter.

[sqwishy]

@ethanhs I could. it might simplify the test a bit. But then it wouldn't run if someone didn't have a Python 2. Let me know if you'd prefer that over one of the previous revisions I made for tests.

[ethanhs]

Okay, I think this looks good to me, thank you for fixing this!

[ethanhs]

If there are no objections I will merge this later today (or someone else can go ahead).

[ilevkivskyi]

It looks like this PR broke PEP 561 packages because previously when checking Python 2 code, mypy looked for stub packages installed in the place where mypy is installed. But after this PR it only looks for PEP 561 packages in Python 2 site packages when type-checking Python 2 code.

I am not sure what PEP 561 says about this, but the previous behavior was intuitive an useful.

[sqwishy]

@ilevkivskyi I'm probably misunderstanding but it sounds like you're saying you have mypy stub python packages installed in the system's python 3 site-packages but you want them available in a python 2 virtualenv? I'd imagine you would want to install those typing stubs along side the package they're for in the virtualenv, right?

[ilevkivskyi]

A (very) common scenario is where you have a CI server that type-checks some Python 2 repository with mypy. This server might not even have Python 2 available (some containers in Travis CI for example). The only reliable way to install PEP 561 stub packages in such scenario is to install them in the same place where mypy is installed. Before this PR this worked perfectly, but with the PR it doesn't.

In addition to this problem, typeshed CI is currently broken, so they can't merge any PRs (the root cause may however be the same).

I don't want to say the PR is bad, I just want to say we need to carefully discuss what do here. We can re-apply when we will have a clear way forward.

@ethanhs What do you think about this?

[JukkaL]

I just want to say we need to carefully discuss what do here.

Any changes that break a common use case should be discussed in an issue, not only in a PR. Also, I don't think that the PR title and description quite describe what's changed. It looks to me like there are two separate changes in this PR, and if that's the case, there should also be two PRs (and two issues).

[sqwishy]

It looks to me like there are two separate changes in this PR

Yeah that's kind of true.

a. Running mypy -2 doesn't behave like mypy --python-version 2.7, this was addressed by setting argument dest to match at the end of the diff in c426585. (I had intended to do that change in a separate commit but forgot sorry.)

b. The other thing is that the site-packages used is different if you set the python version 2.7 in a mypy.ini file compared to specifying it on the command line, because IIRC infer_python_version_and_executable() will use the current interpreter version in the former instead of a Python 2.7 interpreter. The rest of the changeset referenced above addresses that. There other parts of the branch being merged is just a test for this (and not the previous behaviour stated above).

So the confusion seems to be that this second behaviour (b) is a bug to me but a feature to @ilevkivskyi and anybody else who might enjoy getting type information from PEP 561 packages in Python 3/the current interpreter's site-packages when type checking a Python 2 project (if I understand correctly?)

@JukkaL If you'd like. I be glad to open a new PR for just the change to argument parsing (a) to address #5576. And create a new issue here for behaviour (b).

[JukkaL]

I be glad to open a new PR for just the change to argument parsing (a) to address #5576. And create a new issue here for behaviour (b).

Sounds good! (b) clearly is a somewhat tricky issue and worth some more thought.

[sqwishy]

@JukkaL I've created issue #5620 for the problem about using Python 2 site-packages. And pull request #5619 to just adjust the argument handling.