Skip to content

Allow setting an annotation for attr.ib #162

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

Closed
wants to merge 2 commits into from

Conversation

DRMacIver
Copy link

@DRMacIver DRMacIver commented Mar 4, 2017

This is a somewhat speculative pull request to gauge interest. It adds an additional annotation parameter to attr.ib for use with Python 3 type annotations.

Currently this only has two effects:

  • It adds types to __init__
  • It exposes the annotation on the attribute object itself

It would be nice to propagate this further, but there isn't apparently a standard way to do that

My use case is that I'm considering how one might make more things easy to generate out of the box, and having annotations on init helps a lot for that.

Currently this only has two effects:

* It adds types to __init__
* It exposes the annotation on the attribute object itself

It would be nice to propagate this further, but there isn't
apparently a standard way to do that.
@codecov
Copy link

codecov bot commented Mar 6, 2017

Codecov Report

Merging #162 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #162   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         551    561   +10     
  Branches      122    125    +3     
=====================================
+ Hits          551    561   +10
Impacted Files Coverage Δ
src/attr/_make.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5171043...80a5a12. Read the comment docs.

@ambv
Copy link
Contributor

ambv commented Mar 7, 2017

Note that annotation= from your PR might require a different value than a variable annotation on the attribute. Why? Consider my example from the discussion in #151:

class Yeah:
    age = attr.ib(convert=int, default=0)

Here some_yeah.age is always an int but Yeah(age=) accepts anything that converts into an integer. So the annotation= should be at least Union[int, str, bool]. So I'd type the attribute int but the __init__ argument should be a union.

Generally, it's not clear to me what your use case is. There's two issues. No type checker understands annotation=, and its value describes not the type of the attribute but the arguments to __init__.

So in the end I believe an approach that uses regular type annotations will be more practical for the purposes of type checking.

@Tinche
Copy link
Member

Tinche commented Mar 26, 2017

After some contemplation, I think this approach is basically complimentary to #151 and am OK with it being merged in.

My idea was to take the variable annotation and stick it in Attribute.annotation, and for that to happen Attribute.annotation needs to exist first, which this accomplishes. And if Attribute.annotation exists, we would probably want to expose this to earlier versions of Python, which again is accomplished by this PR.

Now, another thing accomplished here is setting the annotations on the generated __init__. We can be more clever here. If an attribute has no converter, the attribute annotation can be used for __init__. If there is a converter present, we could try getting the appropriate annotation from the converter signature. This is the most correct approach.

The problem with this is a lot of Python builtins have no type signatures, so inspecting the converter will fail for easy cases like convert=int. In that case, I dunno, I guess use the attribute annotation.

I have also considered whether creating another Attribute attribute is correct, or whether the existing Attribute.metadata should be used. I think using Attribute.annotation is correct since this is a Python feature we're exposing in a more convenient way.

@hynek
Copy link
Member

hynek commented Mar 27, 2017

I’ll be staying mostly out of these discussions because I’m still not up to speed but I’d like to state for posterity that metadata shouldn’t be used by us at all. Let’s keep a boundary here.

@hynek
Copy link
Member

hynek commented Jul 29, 2017

So this is basically #215 just with different field names, right? I think I’m going to close this if I understand correctly.

@DRMacIver
Copy link
Author

sorry, I haven't had much time to give to this recently. I've no problem with you closing it.

@hynek
Copy link
Member

hynek commented Jul 29, 2017

That wasn't my question but I’ll go ahead. Thanks for contributing mindshare to this non-trivial and emerging topic.

@hynek hynek closed this Jul 29, 2017
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