option to merge stubs into source trees

[gaborbernat]

Resolves #5028.

Need to clean up, both logic mostly there (tested with https://github.com/gaborbernat/tox/tree/type-hint-stubs/tox).

[JelleZijlstra]

I left a couple of stylistic comments.

[gvanrossum]

I think this would need a flag to enable it; I expect that not everybody will want this functionality.

This PR probably ought to be merged quickly to avoid running into merge conflicts; but it requires very thorough review (probably by Jukka or Ivan).

[gaborbernat]

@gvanrossum fair enough, I'll hide it behind a flag.

[JukkaL]

I can review this later this week. Agreed that we want to merge this quickly -- even if this is not 100% finished, if it's behind a flag and doesn't risk breaking other functionality we can hopefully merge it, and it can be iterated on afterwards.

[gaborbernat]

Running works fine. Debugging though generates this extra attached output which breaks the tests. Probably should reformulate to debugging instead running.

…

On Thu, Jun 7, 2018, 19:47 Ethan Smith ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mypy/test/testcmdline.py <#5139 (comment)>: > @@ -58,6 +58,14 @@ def test_python_cmdline(testcase: DataDrivenTestCase) -> None: outb = process.stdout.read() # Split output into lines. out = [s.rstrip('\n\r') for s in str(outb, 'utf8').splitlines()] + + is_running_in_py_charm = "PYCHARM_HOSTED" in os.environ How are you running things from PyCharm? I've been able to run the test suite for months without issue. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAqIPkEtNylTSnLOQsaib9nJuPltkn7qks5t6XUmgaJpZM4UYCqM> .

[JukkaL]

I did a quick overview review pass. The general approach looks reasonable. Thanks for working on this -- this has been requested several times.

I suspect that there are various remaining edge cases, some of which may be a bit tricky (conditional definitions, for example). This just means that this feature needs some real-world use and iteration before it will be generally useful, most of which can happen in separate PRs.

Here's my suggestion for the next steps:

1. Get tests to pass and do some minimal clean-up such as separating unrelated changes from this PR.
2. A core team member (probably me) will try this out and do a more detailed review. The review can be pretty lenient since this is a big feature and we can't expect this to be finished in a single PR.
3. Merge this PR. At this point the feature may be undocumented/hidden, depending on how mature we feel this is.
4. Create follow-up issues for additional work. [Are you willing to continue working on this after a basic implementation gets merged, by the way? If yes, it's easier to justify merging an incomplete feature.]
5. After some iteration make this public as an experimental feature and invite other mypy users to try this out and send feedback.

Finally, the PR doesn't quite follow the style used elsewhere in the codebase. Are you okay with changing the style? At least some of this can happen after this has been merged.

[gaborbernat]

Hello @JukkaL,

1. Many of the changes unrelated to this pr I've done to help me ease the development/setup. Once we want to merge it I can easily remove it. It already sits on its own individual comment exactly for this reason. So sure👌
2. If core members can think of edge cases not covered now I'm open to handle/add them even part of this pr.
3. I believe hidden behind a feature flag we can make it experimental, meaning we documentat it but don't mark it mature yet.
4. I'm committed to finishing and maintaining this. I want to use this feature extensively in my projects, so might as well own it throughout.
5. Sounds good.

As far as style goes whatever you guys want. For all new files I've used the excellent fully automated black formatter with isort import ordering mostly because I prefer spending my mental power on the implementation difficulties rather than how to indent/space my code. That being said I'm not married or attached to any style on my side. Ideally we would have this change automated but if not possible I can alter manually here and there.

[gvanrossum]

A suggestion: if changes unrelated to this topic would benefit others developing mypy, why not submit then as separate PRs? Once accepted you can merge and they won't be part of this PR yet. (Of course these need to be things that don't get in the way of using mypy or other development workflows -- checking for environment variables might be useful here.)

[gaborbernat]

@gvanrossum that would be great and I plan to do that. I don't completely understand what's the current development workflow used by core devs, so my changes don't get in way. Those can be probably discussed inside that pr though.

[gvanrossum]

Mostly we don't use PyCharm much. :-)

[gaborbernat]

#5189 now contains the changes unrelated to this PR.

[gvanrossum]

OK, both Jukka and I are on vacation, so be patient...

(I tried to send this before but somehow the mail bounced.)

[gaborbernat]

No worries I still need to work a bit on this before review/merge time. By the way how do we handle the lack of Type on Python 3.5 and less. Needed to type hint the code of this PR.

[ethanhs]

@gaborbernat you can do:

MYPY=False
if MYPY:
    from typing import Type

And then string escape Type when you use it.

[gaborbernat]

@JukkaL picking this up again, so what blockers we need to get this merged? 👍

[JukkaL]

Based on a quick review pass, these may be sufficient to get this merged:

• Fix remaining failing tests.
• Make style consistent with project style in previously existing files at least, and preferably everywhere (in particular, multi-line function signatures).
• Add at least one command line test that does an end-to-end source/stub merge (in pythoneval.test, probably).
• Get module discovery reviewed in detail, since it has changes to existing functionality. @msullivan may have the most context on this.
• Maybe refactor the new pytest-style tests to use unittest-style test classes, for consistency.
• One of the core team members should do some manual testing and a final round of review. Fix any big enough issues found.

It would also be good to have some commitment for additional work, since this is a pretty big feature. As discussed above, there will likely be various features and edge cases that would still need more work after this PR.

[JukkaL]

We don't use this style for multi-line signatures. The original signature conforms to our style.

[JukkaL]

Another signature with inconsistent style.

[JukkaL]

Can you put these tests inside a test class derived from unittest.TestCase, for consistency?

[gaborbernat]

yes, will do 👍

[JukkaL]

Sorry, I've been busy with other things to finish the review. If you are still interested in finishing this up, I can dedicate some time to reviewing this in May. (Or maybe during PyCon sprints, in case you are coming there.)

[gaborbernat]

@JukkaL yeah I still plan to finish it, will try to make it in some merge-able state by the sprint is my current plan (and yes I'll be there 😄 - I actually have a talk accepted about mypy 🤔 )