Add attrs library #1838

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

third_party/2and3/attr/__init__.pyi

+
+# _make --
+
+NOTHING : object


PEP 8 requires no space before colon.

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

third_party/2and3/attr/__init__.pyi

+
+# 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:

def fun(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

third_party/2and3/attr/__init__.pyi

+_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

third_party/2and3/attr/__init__.pyi

+           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

third_party/2and3/attr/__init__.pyi

+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

third_party/2and3/attr/__init__.pyi

+_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

third_party/2and3/attr/__init__.pyi

+           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

third_party/2and3/attr/__init__.pyi

+           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

third_party/2and3/attr/__init__.pyi

+
+# 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:master Feb 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add attrs library #1838

Add attrs library #1838

chadrik commented Jan 21, 2018

ilevkivskyi left a comment •

edited

Loading

ilevkivskyi Jan 21, 2018

ilevkivskyi Jan 21, 2018

chadrik Jan 21, 2018

ilevkivskyi Jan 21, 2018

chadrik commented Jan 26, 2018

ilevkivskyi commented Jan 26, 2018

chadrik commented Jan 26, 2018

euresti commented Jan 26, 2018

ilevkivskyi commented Jan 26, 2018

JelleZijlstra left a comment

JelleZijlstra Jan 27, 2018

chadrik Jan 27, 2018

euresti Jan 31, 2018

JelleZijlstra Feb 1, 2018

chadrik Feb 2, 2018

JelleZijlstra Jan 27, 2018

chadrik Jan 27, 2018

euresti Jan 31, 2018

JelleZijlstra Feb 1, 2018

JelleZijlstra Jan 27, 2018

chadrik Jan 27, 2018

euresti Jan 31, 2018

chadrik Feb 2, 2018

JelleZijlstra left a comment

JelleZijlstra Feb 1, 2018

JelleZijlstra Feb 1, 2018

JelleZijlstra Feb 1, 2018

JelleZijlstra Feb 1, 2018

chadrik commented Feb 2, 2018

JelleZijlstra commented Feb 2, 2018

gvanrossum commented Feb 3, 2018 via email

Add attrs library #1838

Add attrs library #1838

Conversation

chadrik commented Jan 21, 2018

ilevkivskyi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadrik commented Jan 26, 2018

ilevkivskyi commented Jan 26, 2018

chadrik commented Jan 26, 2018

euresti commented Jan 26, 2018

ilevkivskyi commented Jan 26, 2018

JelleZijlstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelleZijlstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadrik commented Feb 2, 2018

JelleZijlstra commented Feb 2, 2018

gvanrossum commented Feb 3, 2018 via email

ilevkivskyi left a comment •

edited

Loading