Skip to content

Typeshed #884

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

Merged
merged 14 commits into from
Oct 17, 2015
Merged

Typeshed #884

merged 14 commits into from
Oct 17, 2015

Conversation

matthiaskramm
Copy link
Contributor

This removes the stubs/ directory, and adds the "typeshed" submodule to replace it. The directory layout of the latter is quite similar, so the changes to default_lib_path are straightforward.
Note that the submodule currently is using the "mypy" branch of in typeshed, which has, as a subset, the previous contents of stubs/, but sorted into the typeshed directory hierarchy.

This is for #882. It depends on #883.

@o11c
Copy link
Contributor

o11c commented Sep 30, 2015

This almost certainly requires the changes in #721 to actually run the testsuite properly.

@matthiaskramm
Copy link
Contributor Author

I wouldn't count out Jukka yet. He published a pretty comprehensive document on duck-typing just a few weeks ago.

I like the simplification you did to setup.py. And yes, merging our two branches sounds like a great idea. (So that Jukka can, if he wants to, just apply one giant merged pull request.)

What's the HEAD of all your branches? o11c/mypy:master?

@o11c
Copy link
Contributor

o11c commented Sep 30, 2015

3 weeks since any activity isn't a healthy sign, and 3 months that my first PR hasn't been merged ...

I update master only when I think of it, i.e. when I need it in order to use mypy as a pip dependency for one of my other projects. It likely contains outdated copies of branches that I have force-pushed. This purpose is compatible with becoming the community's merge-center-in-exile, though the first step will necessarily be hard-resetting it back to JukkaL's version.

driver is the only branch of mine that really needs to be merged, and all of my other branches depend on it. Since it contains critical fixes to test infrastructure, it should possibly be merged even before yours, even though that might be painful. Otherwise, you might accidentally ship code with tests that fail. This may require some effort to rebase now.

xml-report is complete and should be easily mergeable once driver's conflicts are fixed, though the stubs do need to be moved. I actually created it before driver, but switched them around due to the importance.

syntax is still in progress (I am discovering a handful of bugs as I write test cases for all the trees), though the dialect stuff doesn't depend on anything else.

I've added you as a collaborator to my repo. Feel free to push new branches or rebase my non-active branches (i.e. not syntax right now).

Suggested merge order:

@refi64
Copy link
Contributor

refi64 commented Oct 1, 2015

@o11c Don't worry. @JukkaL's been super busy; this isn't the first time PRs wait for a while before being merged.

@matthiaskramm
Copy link
Contributor Author

@o11c: Sounds good! I'll look into merging my changes into your branches.

@o11c
Copy link
Contributor

o11c commented Oct 1, 2015

@matthiaskramm I've reset started merging stuff into master, and rebased driver in preparation but there are ~20 new test failures I won't investigate tonight.

Do you use IRC? I've registered ##mypy on FreeNode and sent Travis notifications there.

@matthiaskramm
Copy link
Contributor Author

I started playing around with merging the typeshed branch into o11c/mypy:driver (from PR #721), but the latter seems to currently conflict with JukkaL/mypy:master, so I didn't get very far using standard git merge and git rebase. I'll try again by creating a patch from my PR and applying it onto o11c/mypy/driver, hopefully later today.
Btw. I'll be out of town the next week, 10/3 - 10/11.

@o11c
Copy link
Contributor

o11c commented Oct 1, 2015

Er sorry, I pushed it as driver-rebased until I fix the testsuite failures.

@o11c
Copy link
Contributor

o11c commented Oct 1, 2015

I've fixed the testsuite failures, you should be able to rebase o11c/typeshed on top of o11c/driver now.

Make sure you actually add all the stubs, .

Note that python2 support is still incomplete per #732 so only worry about making sure all the python3 stubs for the the current/lower versions are checked for any given run.

@o11c o11c mentioned this pull request Oct 1, 2015
@matthiaskramm
Copy link
Contributor Author

Applied the stubs changes of #721 onto typeshed/, and updated the submodule here.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 7, 2015

The reports of my death have been greatly exaggerated :-)

Due to Life I've been neglecting mypy recently but I'm back now. Apologies for eveybody who's been affected! I'll look into this later this week.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 12, 2015

I've used submodules a little and don't hate them but I also didn't really like them when I used them the last time, and I know a lot of people who will not want to touch them. My main reason for being skeptical is that a lot of people hate them, not that they are actually so bad. I want mypy to have a great developer experience for new and existing contributors, even for those who aren't git experts.

I'd like to see how this affects different mypy contributor workflows. This won't affect normal users as they will use pip to install mypy.

Specific potential issues:

  • Just cloning the repo as normal would no longer produce a working repo? A new contributor would have to first initialize submodules, or use non-default clone options. What will happen if they try to run tests without first initializing submodules?
  • How will rebases and merges work, if two branches have both touched the submodule pointer? Especially for somebody who's never touched submodules before. Based on a my understanding this would result in a conflict, which is not optimal. This would make it a little harder to work on long-living branches.
  • If a branch requires changes to both stubs and other code, the contributor would have to first create a pull request targeting typeshed, wait for that to get merged, and create another pull request targeting mypy with the updates typeshed pointer. This might also bring other, unrelated changes from typeshed, which might cause trouble.
  • Mypy may want to have some local changes to stubs from time to time (e.g., to work around bugs, or to take advantage of nonstandard features). We could use a mypy branch on typeshed, but then contributing to typeshed would be more difficult (as somebody would have to merge or cherry-pick the commit, or the pull request would have to target a branch) when there is a commit that touches both code and stubs.

I have a draft suggestion to an alternative workflow which may or may not be better. Note that here I'm mostly concerned about mypy contributors (including me), as people only using typeshed would see no difference. I'll add this to #882.

@o11c
Copy link
Contributor

o11c commented Oct 12, 2015

I'd like to see how this affects different mypy contributor workflows. This won't affect normal users as they will use pip to install mypy.

Does pip install git://... include submodules? If not we can probably hack the extra clone into setup.py

Just cloning the repo as normal would no longer produce a working repo? A new contributor would have to first initialize submodules, or use non-default clone options. What will happen if they try to run tests without first initializing submodules?

If user runs setup.py or runtests.py, automatically run git submodule update --init.

How will rebases and merges work, if two branches have both touched the submodule pointer? Especially for somebody who's never touched submodules before. Based on a my understanding this would result in a conflict, which is not optimal. This would make it a little harder to work on long-living branches.

Document that people outside of the core contributors should not touch the submodule pointer. There's little danger in running a newer-than-submodule-pointer version of the submodule.

In my experience, the stubs are finished before the main PR, but if not, just comment that in the PR. You have merge permission on typeshed I assume?

If a branch requires changes to both stubs and other code, the contributor would have to first create a pull request targeting typeshed

This makes the workflow easier, as evidenced by the fact that I wrote #903.

, wait for that to get merged, and create another pull request targeting mypy with the updates typeshed pointer.

No, as said earlier.

This might also bring other, unrelated changes from typeshed, which might cause trouble.

If they do, that's a bug, we fix that bug (we should be updating the submodule pointer very frequently anyway), they rebase on master, the bug goes away.

Mypy may want to have some local changes to stubs from time to time (e.g., to work around bugs, or to take advantage of nonstandard features).

If we do have local stubs (which I generally recommend against), we should just keep them in a stubs/3.2 dir and put that in front of typeshed. But I recommend against implementing the infrastructure for this yet, because it's better not to let anyone be tempted to use it.

@matthiaskramm
Copy link
Contributor Author

My main reason for being skeptical is that a lot of people hate them, not that they are actually so bad.

submodules have gotten much better. People still hate them, but they hate them for what they were, not what they're now. They'll lose their bad reputation at some point.

Just cloning the repo as normal would no longer produce a working repo? A new contributor would have to first initialize submodules, or use non-default clone options. What will happen if they try to run tests without first initializing submodules?

"git clone --recursive" will check out submodules, too. But yes, any executables in mypy should warn you if your mypy clone is incomplete (and tell you what git commands to run), rather than failing with an obscure error. I can start a PR on that.

How will rebases and merges work, if two branches have both touched the submodule pointer? Especially for somebody who's never touched submodules before. Based on a my understanding this would result in a conflict, which is not optimal. This would make it a little harder to work on long-living branches.

You'll touch the submodule in order to upgrade to the latest version of typeshed. You'll get a conflict if you're merging two branches that both did that. The standard resolution would be to run "cd typeshed; git checkout master; git pull; cd ..; git add typeshed", in order to upgrade to the latest version of typeshed in the merged branches as well.

If a branch requires changes to both stubs and other code, the contributor would have to first create a pull request targeting typeshed, wait for that to get merged, and create another pull request targeting mypy with the updated typeshed pointer.

That's the correct thing to do. Regardless of the typeshed integration approach we take, we want people to do two separate commits, one for the stubs, one for the rest. Clumping those two changes into the same commit would mean that a core developer has to tease them apart again (in order to sync between typeshed and mypy). It's better if there's a very obvious distinction between modifying the stubs and modifying the code, and it's good that the contributor has to deal with that distinction, not the repository owner. (Better parallelism!)

This might also bring other, unrelated changes from typeshed, which might cause trouble.

Valid concern. But troublesome changes to typeshed will cause problems later, anyway. It might be preferable if they surface as soon as possible.

Mypy may want to have some local changes to stubs from time to time (e.g., to work around bugs, or to take advantage of nonstandard features).

I have a feeling that files like builtins.py or collections.py will occasionally need local changes. The other stubs, not so much. As mentioned in my other comment, we can have an "override/" directory in mypy that contains mypy-specific stubs, with the understanding that it should not be overused.
(And we probably want the "override/" directory regardless of whether we use submodules, subtrees or manual copying, since if mypy-specific changes are applied directly onto "stubs/", syncing would become very hard.)

@o11c
Copy link
Contributor

o11c commented Oct 13, 2015

Suggest typeshed be added as a subdirectory of mypy so that we can use package_data instead of data_files in setup.py.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 14, 2015

I played around with this and looks mostly good (together with #923). [Though I'm starting to get lost in the tangles of git history. Maybe we should move to a more linear, rebase-based history.]

Some things I noticed:

  • The latest master contains a critical fix to module search path logic. Probably worth testing that it doesn't break anything here.
  • python3 setup.py install doesn't seem to install stubs. This may fix it (together with merging master): https://gist.github.com/JukkaL/9bc9f218dfd9ebcd2677
  • We need some documentation about the typeshed submodule, probably in the readme and elsewhere.

@matthiaskramm
Copy link
Contributor Author

Merged master, and fixed the path in setup.py (by just doing find_data_files('typeshed', ['*.py', '*.pyi']) - am I missing a corner case here that's addressed by your snippet?)

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 15, 2015

My snippet was adapted from an earlier commit -- I didn't write it myself. I first tried to do what you did now, but I hadn't merged master so it was still failing. Then I tried a few different things, including merging master, but didn't get back to trying the original fix attempt. I believe that find_data_files works, but I'll go through it once more.

I don't have time to test this today, but I'll try to get back to this ASAP.

@JukkaL JukkaL merged commit 040bcd9 into python:master Oct 17, 2015
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.

4 participants