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

Support factory= in attr plugin. #5005

Merged
merged 3 commits into from May 7, 2018

Conversation

Projects
None yet
2 participants
@euresti
Copy link
Contributor

euresti commented May 4, 2018

This is a new feature in attrs 18.1
The implementation is just like default=

Support factory= in attr plugin.
It works pretty much like default=
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! Here are some comments.

attr_has_factory = bool(_get_argument(rvalue, 'factory'))

if attr_has_default and attr_has_factory:
ctx.api.fail("Can't pass both `default` and `factory`.", rvalue)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 7, 2018

Collaborator

It is a good idea to always add tests for any newly added error messages.

@attr.s
class A:
x: List[int] = attr.ib(factory=list)
y: int = attr.ib(factory=my_factory)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 7, 2018

Collaborator

Please add also a test where this fails due to a wrong return type of the factory function.

if attr_has_default and attr_has_factory:
ctx.api.fail("Can't pass both `default` and `factory`.", rvalue)
elif attr_has_factory:
attr_has_default = attr_has_factory

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 7, 2018

Collaborator

I think attr_has_default = True would be shorter and cleaner.

@euresti

This comment has been minimized.

Copy link
Contributor Author

euresti commented May 7, 2018

Done and done!

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Few more comments.

factory: Optional[Callable[[], _T]] = ...,
) -> _T: ...
# This form catches explicit None or no default but with no other arguments returns Any.
# This form catches explicit None or no default but with no other arguments returns Any.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 7, 2018

Collaborator

This line is repeated twice.

This comment has been minimized.

@euresti

euresti May 7, 2018

Author Contributor

Oops.


_T = TypeVar('_T')
_C = TypeVar('_C', bound=type)

_ValidatorType = Callable[[Any, Any, _T], Any]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 7, 2018

Collaborator

I thought validator should return bool, isn't this the case?

This comment has been minimized.

@euresti

euresti May 7, 2018

Author Contributor

The return value of validators is ignored. A validation error is a raised exception. However I don't want to force a "None"

import attr
@attr.s
class A:
x: int = attr.ib(factory=list) # E: Incompatible types in assignment (expression has type "List[T]", variable has type "int")

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 7, 2018

Collaborator

The error message looks a bit cryptic. Why it is List[T] and not List[Any]? There is no T in code. I suppose this might be a bug in using class as factory, is the error better if you use a generic function instead of class?

This comment has been minimized.

@euresti

euresti May 7, 2018

Author Contributor

This error come from the stubs not from the plugin (i.e. any method that takes a type would give this error message.) I can add a generic function too.

CR
@euresti

This comment has been minimized.

Copy link
Contributor Author

euresti commented May 7, 2018

Thanks for the quick review. Your requests have been implemented.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

OK, if there are no concerns, I am going to merge this later today.

@ilevkivskyi ilevkivskyi merged commit bb13ecc into python:master May 7, 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
You can’t perform that action at this time.