Skip to content

Support types as converters in attrs plugin #4917

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

Merged
merged 7 commits into from
May 4, 2018

Conversation

euresti
Copy link
Contributor

@euresti euresti commented Apr 16, 2018

Fixes #4729

@chadrik
Copy link
Contributor

chadrik commented Apr 16, 2018

This unblocks: python-attrs/attrs#238

Copy link
Member

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

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

Thank you for adding this! I went ahead and changed the type annotation to a type comment and I left a few requests w.r.t. the tests.

[file a.py]
from typing import overload
import attr
class complex:
Copy link
Member

Choose a reason for hiding this comment

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

Especially since it is duplicated across several test cases, moving this to a fixture would be good.

from typing import overload
import attr

class complex:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should be moved to a fixture?

class complex:
@overload
def __init__(self, re: float = 0.0, im: float = 0.0) -> None: ...
@overload
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add tests for an overloaded converter function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this note. It was actually broken!

@euresti
Copy link
Contributor Author

euresti commented Apr 21, 2018

Thanks for the review! I've addressed all your concerns and fixed some more things that were broken.

@euresti
Copy link
Contributor Author

euresti commented May 4, 2018

@ethanhs is there a way to re-request review that I'm missing here?

@emmatyping
Copy link
Member

Thank you again for working on this @euresti !

@gvanrossum gvanrossum merged commit f97b98e into python:master May 4, 2018
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