Add attrs library by chadrik · Pull Request #1838 · python/typeshed

chadrik · 2018-01-21T06:10:06Z

Hi, here are stubs for attrs. I've already received permission from @hynek here.

After this is merged, @euresti will follow up with his PR on mypy to add plugin support.

ilevkivskyi

Just two style comments.

ilevkivskyi · 2018-01-21T09:50:23Z

+
+# _make --
+
+NOTHING : object


PEP 8 requires no space before colon.

ilevkivskyi · 2018-01-21T09:52:17Z

+
+# 1st form catches _T set.
+@overload
+def attrib(default: Optional[_T] = ..., validator: Optional[_ValidatorArgType[_T]] = ..., repr: bool = ..., cmp: bool = ..., hash: Optional[bool] = ..., init: bool = ..., metadata: Mapping = ..., type: Optional[Type[_T]] = ..., converter: Optional[_ConverterType[_T]] = ...) -> _T: ...


You can split this definition one parameter per line:
deffun(x: type_x, y: type_y, z: type_z) ->ret_type: ...

You can split this definition one parameter per line:
I'm happy to oblige, but I'm having a little trouble reconciling that advice with the guidelines in CONTRIBUTING.md:
there is no line length limit;
prefer long lines over elaborate indentation;
If there is no line length limit, how do I decide which definitions should be split per line? This also seems to go against the latter, preferring long lines to indentation.

There are no strict rules, you should just apply your reason, as an example there is builtins.open in Python 3.
My rule of thumb is if it doesn't fit in one line on my Mac, then it is definitely too long :-)

chadrik · 2018-01-26T01:15:00Z

@ilevkivskyi What's next?

ilevkivskyi · 2018-01-26T01:29:30Z

The mypy PR you mentioned is higher priority for me than this (and Python 3.7 beta 1 in four days is even higher priority).

chadrik · 2018-01-26T02:34:26Z

@euresti, who is doing the mypy PR, has indicated that he needs this merged to get his PR passing tests and mergeable.

euresti · 2018-01-26T05:19:09Z

Technically, without the stubs the plugin would never be called. (I haven't looked into why I just tested it and it doesn't get called.)

ilevkivskyi · 2018-01-26T09:15:53Z

Technically, without the stubs the plugin would never be called.

This is because mypy currently only uses only typeshed. After PEP 561 is implemented it will also accept inline annotations and side-by-side stubs in third party packages.

JelleZijlstra

Don't have time to review all this against the attrs implementation right now, but left a few comments.

JelleZijlstra · 2018-01-27T00:04:35Z

+_ValidatorType = Callable[[Any, Attribute, _T], Any]
+_ConverterType = Callable[[Any], _T]
+_FilterType = Callable[[Attribute, Any], bool]
+_ValidatorArgType = Union[_ValidatorType[_T], List[_ValidatorType[_T]], Tuple[_ValidatorType[_T], ...]]


Don't have time to read through the attrs code right now, but why isn't this just Iterable or Sequence? Does it really accept only lists and tuples?

The code literally does isinstance(validator, (list, tuple)):

That's unfortunate because list and tuple are invariant, so subtypes of _ValidatorType may end up not getting accepted.

Good point. Confirmed that I'm getting errors with lists of validators. I'm going to use Sequence for now and I'll open a separate ticket against attrs to see if this can be addressed in the code.

JelleZijlstra · 2018-01-27T00:04:49Z

+ cmp: bool = ...,
+ hash: Optional[bool] = ...,
+ init: bool = ...,
+ metadata: Mapping = ...,


Missing type arguments to Mapping.

attrs doesn't have any opinion or requirement about the types within the mapping.

It should probably be Optional[Mapping[Any, Any]] since None is acceptable

In general, even if the type arguments are Any I prefer being explicit, so that we can detect cases where we skip type arguments by accident.

JelleZijlstra · 2018-01-27T00:06:06Z

+def get_run_validators() -> bool: ...
+
+# aliases --
+# s = attributes = attrs


Why can't you just write these lines instead of duplicating the definitions? Does that interact poorly with overloads?

Just below those commented lines:
# FIXME: there is a bug in PyCharm with creating aliases to overloads. # Remove these when the bug is fixed: # https://youtrack.jetbrains.com/issue/PY-27788

I would take out the commented out assignments entirely. Just to avoid confusion.

I find it's nice when testing to be able to uncomment them. I'll move the FIXME to a more co-located place.

JelleZijlstra

A few more minor comments, then this should be good to go.

JelleZijlstra · 2018-02-01T23:46:43Z

+_ValidatorType = Callable[[Any, Attribute, _T], Any]
+_ConverterType = Callable[[Any], _T]
+_FilterType = Callable[[Attribute, Any], bool]
+_ValidatorArgType = Union[_ValidatorType[_T], List[_ValidatorType[_T]], Tuple[_ValidatorType[_T], ...]]


That's unfortunate because list and tuple are invariant, so subtypes of _ValidatorType may end up not getting accepted.

JelleZijlstra · 2018-02-01T23:51:28Z

+ cmp: bool = ...,
+ hash: Optional[bool] = ...,
+ init: bool = ...,
+ metadata: Mapping = ...,


In general, even if the type arguments are Any I prefer being explicit, so that we can detect cases where we skip type arguments by accident.

JelleZijlstra · 2018-02-01T23:53:55Z

+ init: bool = ...,
+ metadata: Mapping = ...,
+ type: Optional[Type[_T]] = ...,
+ converter: Optional[_ConverterType[_T]] = ...) -> _T: ...


The convert argument is missing. It is apparently deprecated, but we should still include it because otherwise the positional arguments are out of sync. We could perhaps mark some or all of the arguments as keyword-only here (it will work even in Python 2).

JelleZijlstra · 2018-02-01T23:56:18Z

+
+# TODO: add support for returning a proper attrs class from the mypy plugin
+# we use Any instead of _CountingAttr so that e.g. `make_class('Foo', [attr.ib()])` is valid
+def make_class(name, attrs: Union[List[str], Dict[str, Any]], bases: Tuple[type, ...] = ..., **attributes_arguments) -> type: ...


missing type (presumably Any) on attributes_arguments

- add attrib(convert) - replace Mapping with Optional[Mapping[Any, Any]] - add make_class arg types - deal properly with sequences of validator types - fix a bug with forward references

chadrik · 2018-02-02T20:05:49Z

review notes addressed.

Btw, I've added pretty thorough tests to my attrs PR for pyi stubs which cover both the attrs stubs and mypy plugin.

JelleZijlstra · 2018-02-02T23:15:18Z

Thanks for addressing the comments! There's a minor lint error now.

gvanrossum · 2018-02-03T05:44:07Z

Wheeee!!!

…

On Feb 2, 2018 9:26 PM, "Jelle Zijlstra" ***@***.***> wrote: Merged #1838 <#1838>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1838 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACwrMiR6pNDBB0qQz7GF4iNsJT1y4gMOks5tQ-4NgaJpZM4Rlth8> .

* python/master: (269 commits) allow Optional[float] for asyncio.wait() timeout arg (python#1860) Clean up the pytype blacklist. (python#1851) filter function: make the callable of the first overload non-optional so that mypy is able to select the second overload for the case. (python#1855) Accept Text in distutils *Version classes (python#1854) Add attrs library (python#1838) Add type stub for the lzma module (python#1844) Add ImportError attributes name, path for Python 3.3+ (python#1849) Add 'message' to click.decorators.version_option (python#1845) PEP 553 and PEP 564 (python#1846) Adding py36 additions to datetime module (python#1848) Remove incomplete thrift stubs that cause false positives. (python#1827) adding curses.ascii, curses.panel and curses.textpad modules (python#1792) Fix requests session hooks type (python#1794) Add StringIO.name and BytesIO.name (python#1802) Fix werkzeug environ type (python#1831) Change the return type of unittest.TestCase.fail() to NoReturn (python#1843) _curses.tparm works on bytes, not str (python#1828) Refine types for distutils.version (python#1835) Add stub for stat.filemode (python#1837) modify pytype_test to run from within pytype too, and support python3 (python#1817) ...

Add stubs for attrs library
fc3a8dd

ilevkivskyi reviewed Jan 21, 2018
View reviewed changes

chadrik added 2 commits January 21, 2018 10:30

Address code review notes about style
567e484

Forgot one
c8361cf

JelleZijlstra reviewed Jan 27, 2018
View reviewed changes

JelleZijlstra requested changes Feb 2, 2018
View reviewed changes

euresti mentioned this pull request Feb 2, 2018
Plugin to typecheck attrs-generated classes python/mypy#4397
Merged

Address review notes
916bbb2
- add attrib(convert) - replace Mapping with Optional[Mapping[Any, Any]] - add make_class arg types - deal properly with sequences of validator types - fix a bug with forward references

JelleZijlstra approved these changes Feb 2, 2018
View reviewed changes

fix flakes failure
43abb30

JelleZijlstra merged commit b111a45 into python:masterFeb 3, 2018

Uh oh!

Uh oh!

Conversation

chadrik commented Jan 21, 2018

Uh oh!

ilevkivskyi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chadrik commented Jan 26, 2018

Uh oh!

ilevkivskyi commented Jan 26, 2018

Uh oh!

chadrik commented Jan 26, 2018

Uh oh!

euresti commented Jan 26, 2018

Uh oh!

ilevkivskyi commented Jan 26, 2018

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chadrik commented Feb 2, 2018

Uh oh!

JelleZijlstra commented Feb 2, 2018

Uh oh!

gvanrossum commented Feb 3, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ilevkivskyi left a comment •
edited
Loading