-
-
Notifications
You must be signed in to change notification settings - Fork 365
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 attr.resolve_types #302
Conversation
b5d0f2b
to
3c27e7d
Compare
Codecov Report
@@ Coverage Diff @@
## master #302 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 9 9
Lines 731 741 +10
Branches 152 154 +2
=====================================
+ Hits 731 741 +10
Continue to review full report at Codecov.
|
I'm not sure if I need to implement any Hypothesis things. Please advise. |
Code looks good on a first look but is this really what we should do or is that a hack around something else we should be doing? I’m honestly asking because the whole topic is utterly confusing to me 🤯 and the discussions that happened here didn’t really help. :) |
Sure! So here's what I understand: There are 2 ways to get the type annotations. Currently
In the future, it will just always give you a string, see https://www.python.org/dev/peps/pep-0563/.
Now as far as So I think
So for those in 2, this code would be useful. And just to double check, @Tinche would this code help you at all? |
Before I dive any deeper, how are data classes coping with this, @ericvsmith? |
It's an open issue: ericvsmith/dataclasses#92 The only place where As far as point 2 is concerned, I don't think this is a In an earlier discussion about it (that of course I can't find), I think it was Guido who suggested that if we find a string attribute, just scan the string for I'm deliberately avoiding importing I'm sort of waiting for PEP 563 to be accepted or not before I do anything. If it's accepted, maybe there will be better tools for this. Thanks for pointing me to this issue. |
Thanks for your input! IOW: you just YOLO it for now and check a bunch of string. As a fun aside, we have a gross ClassVar thingie too (for the same reason): https://github.com/python-attrs/attrs/blob/master/src/attr/_make.py#L186 if it makes you feel better. :) |
For now I completely ignore the issue and never treat a string as a Your hack does make me feel better! Whatever we end up doing, I'm hoping we can use the same approach. |
@ericvsmith, PEP 563 is accepted, it's being implemented now, I'll finish this around Christmas time. The only thing left is really f-strings. Unparsing those is not super easy ;-) @euresti, I would much prefer if you didn't reimplement PEP 563 requires updates to |
Would anyone feel bad if we pushed this to 18.1? I would like to get the fixes of 17.4 out and this seems to be still a bit in the air. |
@ambv: I'm not sure what you're trying to do with f-string parsing, but there might be some helpers in the compiler and ast that can help. I wouldn't mind exposing some of them, sort of like |
Delaying is fine, and I'd love to see what happens to get_type_hints. |
How are we gonna proceed here? 😇 |
I'm actually unsure how useful this is. Perhaps this would serve better as separate project, one in which you want runtime types. Maybe part of cattrs or something new. I'm fine with closing this without merge. |
PEP 563 is implemented, |
Based on all the new developments (mypy support, typing.get_type_hints() etc) I'm ok closing this unmerged. Basically all this code does is call |
Alright, thanks again for all the work you put into it! |
Ok this is ready. The only part I'm not super happy about is that my tests pass |
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.
Mostly docs & questions – code looks good to my untrained eye. :)
src/attr/_make.py
Outdated
class. | ||
:raise NameError: If types cannot be resolved because of missing variables. | ||
|
||
.. versionadded:: 17.4.0 |
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.
cough :D
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.
What should it be?
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.
20.1.0 – we're doing CalVer. The next version can always be found in the package's __init__.py
(sans dev suffix).
Very random thought: it would totally be useful if it worked as a class decorator too, no? Like in: @attr.resolve_types
@attr.dataclass
class C:
X: Foo |
Interesting. That will work unless you've got a ForwardRef e.g. This works
But this won't:
But not-too hard to implement. |
This adds `attr.resolve_types` which can be used to resolve forward declarations in classes created using `__annotations__` Fixes python-attrs#265
Add to stubs Make it a decorator, because why not?
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'll fix the version myself, thanks!
This adds
attr.resolve_types
which can be used to resolve forward declarations in classes created using__annotations__
Fixes #265
Pull Request Check List
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!
.rst
files is written using semantic newlines.versionadded
,versionchanged
, ordeprecated
directives.changelog.d
.If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!