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

Infer type annotations from converters #710

Merged
merged 14 commits into from
Dec 13, 2020
Merged

Conversation

nosewings
Copy link
Contributor

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.

docs/init.rst Outdated
>>> @attr.s
... class C(object):
... x = attr.ib(converter=str2int)
... y = attr.ib(converter=str2int, type=int)
Copy link
Member

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.

Copy link
Contributor Author

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:

  1. If inspect.signature finds that the converter doesn't take any arguments, an IndexError 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).
  2. I haven't special-cased pipe, because I don't see a good way to do that without modifying the definition of pipe (e.g., turning it into a callable class).

Copy link
Contributor

@Drino Drino Nov 18, 2020

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.

Copy link
Member

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?

Copy link
Contributor

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_wrapperalso copies signature information (which is important in this case, as annotations are parsed from signature) and just copying __annotations__ wouldn't do the trick.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, why not then.

@hynek hynek added this to the 20.4.0 milestone Nov 5, 2020
@nosewings
Copy link
Contributor Author

I went ahead and implemented inference for pipe and optional (default_if_none isn't possible without knowing the type of the default argument). I don't think the update_wrapper approach will work, because, e.g., the function returned by pipe takes exactly one positional argument named val, not any other arguments that the first wrapped converter might take. Instead, I had to use inspect and copy over the necessary annotations manually.

@nosewings
Copy link
Contributor Author

I suppose the other option would be to make pipe (and optional) take *args, **kwargs; then the update_wrapper approach would make sense (I think).

@hynek
Copy link
Member

hynek commented Dec 13, 2020

@Drino would you mind having a look?

docs/init.rst Show resolved Hide resolved
if not PY2:
if not converters:
# If the converter list is empty, pipe_converter is the identity.
A = typing.TypeVar("A")
Copy link
Contributor

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.

@Drino
Copy link
Contributor

Drino commented Dec 13, 2020

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 *args, **kwargs approach is a little bit more elegant.

Added a couple of pesky comments, but anyway it looks good to me.

@nosewings thank you for the great PR :)

@hynek hynek merged commit e09b1d6 into python-attrs:master Dec 13, 2020
@hynek
Copy link
Member

hynek commented Dec 13, 2020

Thanks everyone!

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