Skip to content

Add stubs for werkzeug #530

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 2 commits into from
Nov 28, 2016
Merged

Add stubs for werkzeug #530

merged 2 commits into from
Nov 28, 2016

Conversation

rowillia
Copy link
Contributor

@rowillia rowillia commented Sep 9, 2016

No description provided.

@matthiaskramm
Copy link
Contributor

I see that you're also doing some stdlib/ changes in your PR. Could you split those into another pull request?

@@ -7,8 +7,9 @@
# NOTE: These are incomplete!

from typing import (
Any, Dict, Generic, TypeVar, Iterable, Tuple, Callable, Mapping, overload, Iterator,
Sized, Optional, List, Set, Sequence, Union, Reversible, MutableMapping, MutableSequence
Any, Container, Dict, Generic, TypeVar, Iterable, Tuple, Callable, Mapping, overload,
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's just a cleanup. Can you roll that back? It's not pertinent to the purpose of the PR, and in general we don't accept cleanup diffs (they just cause churn without much value).

@@ -0,0 +1,110 @@
# Stubs for cookielib (Python 2)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this a separate PR? It's easier to manage things that way.

@@ -13,9 +13,11 @@ from typing import (
# These are exported.
# TODO reexport more.
from typing import (
Container as Container,
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, but I'd like to see this (and anything else that's not strictly part of werkzeug) in a separate PR.

@gvanrossum gvanrossum changed the title Add stubs for werkzeug [has conflicts] Add stubs for werkzeug Sep 23, 2016
@gvanrossum
Copy link
Member

Hey Roy, are you still interested in this?

gvanrossum pushed a commit that referenced this pull request Nov 9, 2016
(Extracted from PR #530 by Roy Williams, @rowilla.)
gvanrossum pushed a commit that referenced this pull request Nov 9, 2016
(Extracted from PR #530.))
gvanrossum pushed a commit that referenced this pull request Nov 9, 2016
@gvanrossum
Copy link
Member

Hey @rowillia are you interested in bringing this to completion? I've cherry-picked your addition of cookielib and your changes to collections and urllib. Also, one reason for the merge conflict is probably that I renamed the 2.7 directories to 2. It may be easier to just copy all the files to a backup tree and start over with a new PR. Or perhaps all you need to do is move the werkzeug stubs.

@gvanrossum gvanrossum dismissed their stale review November 9, 2016 03:45

I was mostly requesting separating the stdlib changes, and I've now cherry-picked those into master.

@gvanrossum gvanrossum changed the title [has conflicts] Add stubs for werkzeug Add stubs for werkzeug Nov 9, 2016
@rowillia
Copy link
Contributor Author

@gvanrossum Rebasing this now. I need #691 in to get this passing in mypy/2.7

@gvanrossum
Copy link
Member

Thanks. I need to take a break from all work, hopefully it can wait or someone else will be able to review it.

@rowillia
Copy link
Contributor Author

@gvanrossum Enjoy your time off!

@rowillia
Copy link
Contributor Author

@gvanrossum Rebased with passing tests now that the MutableSet PR is in. Hope you enjoyed your time unplugging 😄

@gvanrossum gvanrossum merged commit 021b162 into python:master Nov 28, 2016
@gvanrossum
Copy link
Member

Thanks! Is your next project going to be flask stubs?

@rowillia
Copy link
Contributor Author

@gvanrossum it will be 😄

@gvanrossum
Copy link
Member

gvanrossum commented Nov 28, 2016

BTW, this puts the Python 2 files in third_party/2.7/ -- they should be in third_party/2/. See #635 for why. I will fix this in #717.

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