-
-
Notifications
You must be signed in to change notification settings - Fork 383
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
Infer type annotations from converters #710
Conversation
docs/init.rst
Outdated
>>> @attr.s | ||
... class C(object): | ||
... x = attr.ib(converter=str2int) | ||
... y = attr.ib(converter=str2int, type=int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct behavior. The type parameter is equivalent to using a type annotation. IOW: it's what's stored on the class, not what the initializer is accepting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, and I've updated PR to reflect that. It's worth noting, though, that the type
argument now becomes a no-op if a converter is present. I suppose that was already the case, though.
The only issues I see now are:
- If
inspect.signature
finds that the converter doesn't take any arguments, anIndexError
will be thrown at line 2208. In most cases, an error would have occurred anyway -- but it would have happened later, and it would have been more readable (e.g.,f() takes 0 positional arguments but 1 was given
). - I haven't special-cased
pipe
, because I don't see a good way to do that without modifying the definition ofpipe
(e.g., turning it into a callable class).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 - why don't just do update_wrapper
in pipe
?
if not PY2 and converters:
functools.update_wrapper(pipe_converter, converters[0])
Return-type annotation may be inaccurate, but the arguments annotations would anyway flow into __init__
. To fix return ones, something like this can be done:
if 'return' in converters[-1].__annotations__:
pipe_converter.__annotations__['return'] = converters[-1].__annotations__['return']
else:
del converters[-1].__annotations__['return']
but I'm not sure if this approach is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm the update_wrapper is intriguing, but the problem is that it also copies over __name__
etc which would make it confusing.
But I guess we could do just a pipe_converter.__annotations__ = converters[0].__annotations__
and then the handling of return you've outlined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just take only annotations on update_wrapper:
In [2]: def test_me(a: int, b: str): pass
In [3]: def test_wrapper(*args, **kwargs): pass
In [4]: update_wrapper(test_wrapper, test_me, assigned=('__annotations__',))
Out[4]: <function __main__.test_wrapper(a: int, b: str)>
Please, note that update_wrapper
also copies signature information (which is important in this case, as annotations are parsed from signature) and just copying __annotations__
wouldn't do the trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not then.
Merging from main repo before push
I went ahead and implemented inference for |
I suppose the other option would be to make |
@Drino would you mind having a look? |
if not PY2: | ||
if not converters: | ||
# If the converter list is empty, pipe_converter is the identity. | ||
A = typing.TypeVar("A") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't like how it works with empty pipe, as it will put TypeVar("A")
in __init__
annotation. Can't figure a much better solution, though.
I'm OK with parsing only argument type (e.g. it may be better for 3-argument converters when they will be added), though in my personal opinion Added a couple of pesky comments, but anyway it looks good to me. @nosewings thank you for the great PR :) |
Thanks everyone! |
.pyi
).tests/typing_example.py
.docs/api.rst
by hand.@attr.s()
have to be added by hand too.versionadded
,versionchanged
, ordeprecated
directives. Find the appropriate next version in our__init__.py
file..rst
files is written using semantic newlines.changelog.d
.