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

Support types as converters in attrs plugin #4917

Merged
merged 7 commits into from May 4, 2018

Conversation

Projects
None yet
4 participants
@euresti
Contributor

euresti commented Apr 16, 2018

Fixes #4729

@chadrik

This comment has been minimized.

Contributor

chadrik commented Apr 16, 2018

This unblocks: python-attrs/attrs#238

@ethanhs

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:

This comment has been minimized.

@ethanhs

ethanhs Apr 16, 2018

Collaborator

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:

This comment has been minimized.

@ethanhs

ethanhs Apr 16, 2018

Collaborator

Perhaps this should be moved to a fixture?

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

This comment has been minimized.

@ethanhs

ethanhs Apr 16, 2018

Collaborator

Could you also add tests for an overloaded converter function?

This comment has been minimized.

@euresti

euresti Apr 18, 2018

Contributor

Thanks for this note. It was actually broken!

@euresti

This comment has been minimized.

Contributor

euresti commented Apr 21, 2018

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

@euresti

This comment has been minimized.

Contributor

euresti commented May 4, 2018

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

@ethanhs

ethanhs approved these changes May 4, 2018

@ethanhs

This comment has been minimized.

Collaborator

ethanhs commented May 4, 2018

Thank you again for working on this @euresti !

@gvanrossum gvanrossum merged commit f97b98e into python:master May 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment