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

Add basic stub for characteristic #771

Merged
merged 8 commits into from
Dec 24, 2016
Merged

Add basic stub for characteristic #771

merged 8 commits into from
Dec 24, 2016

Conversation

alexjurkiewicz
Copy link
Contributor

@gvanrossum
Copy link
Member

Did you get permission from the authors/developers of the package? See https://www.python.org/dev/peps/pep-0484/#the-typeshed-repo

@hynek
Copy link
Member

hynek commented Dec 15, 2016

Permission granted and please look into using attrs instead. ;)

@gvanrossum
Copy link
Member

OK, but what we have here is incomplete -- it only defines one method. @alexjurkiewicz Can you make it more complete? (Maybe run stubgen or just transcribe the major API elements by hand.

@alexjurkiewicz
Copy link
Contributor Author

Thanks @hynek -- I apologise for my lack of legacy rework :D

I've added the remainder of the public API. How do you want the pyi file formatted? Some of the function definitions are extremely long, I've split them over multiple lines, but other files seem to use a single line...

@gvanrossum
Copy link
Member

This looks fine, but could you reformat using 4-space indents? That aspect of PEP 8 still applies... :-)

whoops!
@alexjurkiewicz
Copy link
Contributor Author

whoops!

@ambv
Copy link
Contributor

ambv commented Dec 19, 2016

@hynek, the irony of the characteristic tagline in this context does not escape me! 😆

@gvanrossum, doesn't the use of class decorators like immutable or with_repr erase types with the current type definition? It seems to me those class decorators should rather be annotated as:

T = TypeVar('T')
def with_repr(attrs: Sequence[Union[AnyStr,Attribute]]) -> Callable[[T], T]: ...

@ambv ambv force-pushed the master branch 10 times, most recently from 63ba0ab to 7853c26 Compare December 23, 2016 00:12
@@ -0,0 +1,34 @@
from typing import Sequence, Callable, Union, Any, Optional, AnyStr, TypeVar, Type

def with_repr(attrs: Sequence[Union[AnyStr,Attribute]]) -> Callable[..., Any]: ...
Copy link
Member

Choose a reason for hiding this comment

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

There should be a space after the comma between AnyStr and Attribute (here and below).

@ambv -- I'm surprised flake8 didn't catch this?

@gvanrossum
Copy link
Member

@gvanrossum, doesn't the use of class decorators like immutable or with_repr erase types with the current type definition? It seems to me those class decorators should rather be annotated as:

T = TypeVar('T')
def with_repr(attrs: Sequence[Union[AnyStr,Attribute]]) -> Callable[[T], T]: ...

Sadly that doesn't work yet: python/mypy#1551

@ambv
Copy link
Contributor

ambv commented Dec 24, 2016 via email

@alexjurkiewicz
Copy link
Contributor Author

alexjurkiewicz commented Dec 24, 2016 via email

ambv referenced this pull request Dec 24, 2016
Starting with python/mypy#2521 mypy is performing stricter function signature
checks.

This makes the stubs diverge from the actual implementation but makes the stubs
internally consistent.  Since this is an actual typing issue in the base
implementation, we need to defer to the original authors to fix it.

Sadly, in this case the breakage is rather fundamental and unlikely to get
fixed by upstream. Consider:

```
  class AWSAuthConnection(object):
    def make_request(self, method, path, headers=None, data='', host=None,
      auth_path=None, sender=None, override_num_retries=None,
      params=None, retry_handler=None): ...

  class AWSQueryConnection(AWSAuthConnection):
    def make_request(self, action, params=None, path='/', verb='GET'): ...
```

Hence, until we have a workaround for the error produced by Mypy, we're
excluding those stubs from being tested against.
@gvanrossum
Copy link
Member

Check it out locally and rebase, you'll see it fail the 'flake8' run.

I had done exactly that, and it passed. Then I found that I didn't run pip3 install -U -r requirements-tests-py3.txt. With that it finds these bugs.

I'm tempted to just fix the whitespace using GitHub's edit feature and merge.

@gvanrossum gvanrossum merged commit 8e9c53f into python:master Dec 24, 2016
@gvanrossum
Copy link
Member

Thanks! If you want to change more, just submit a new PR.

@alexjurkiewicz
Copy link
Contributor Author

alexjurkiewicz commented Dec 24, 2016 via email

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