| msg309065 - (view) |
Author: Eric V. Smith (eric.smith) *  |
Date: 2017-12-26 18:34 |
| For this class: @dataclass class C: x: int = 0 y = 0 Generate a TypeError. 'y' is not a field (as defined by @dataclass). It should either be a regular field (by giving it a type annotation) or a ClassVar field. |
|
|
| msg309071 - (view) |
Author: Ivan Levkivskyi (levkivskyi) * |
Date: 2017-12-26 23:24 |
| A possible question here is should we give an error for any non-callable name in `__dict__` which is not in `__annotations__` or only for `Field`s? After some thinking I am actually leaning towards the first option. |
|
|
| msg309075 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2017-12-27 02:09 |
| I'm not sure I understand the distinction. You have to look through everything in `__dict__`, then exclude those things that are any type of field (so, real fields or pseudo-fields). Those are the things that are in `__annotations__`, anyway. The trick is what else to exclude. In this class: class C: x: int = 0 y = 0 def func(self): pass @staticmethod def staticmeth() : pass @classmethod def classmeth(cls) : pass @property def prop(self): pass These are the non-callables: print([k for k in C.__dict__ if not callable(getattr(C, k))]) ['__module__', '__annotations__', 'x', 'y', 'prop', '__dict__', '__weakref__', '__doc__'] How do we only pick out `y` and probably `prop`, and ignore the rest, without being overly fragile to new things being added? I guess ignoring dunders and things in `__annotations__`. Is that close enough? |
|
|
| msg309112 - (view) |
Author: Ivan Levkivskyi (levkivskyi) * |
Date: 2017-12-28 00:19 |
| > I'm not sure I understand the distinction. Initially I thought about only flagging code like this: @dataclass class C: x = field() But not this: @dataclass class C: x = 42 Now I think we should probably flag both as errors. > How do we only pick out `y` and probably `prop`, and ignore the rest, without being overly fragile to new things being added? I guess ignoring dunders and things in `__annotations__`. Is that close enough? We had a similar problem while developing Protocol class (PEP 544). Currently we just a have a whitelist of names that are skipped: '__abstractmethods__', '__annotations__', '__weakref__', '__dict__', '__slots__', '__doc__', '__module__' (plus some internal typing API names) |
|
|
| msg309189 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2017-12-29 19:20 |
| I liked the original design better, where things without annotations would just be ignored. What changed? On Dec 27, 2017 5:19 PM, "Ivan Levkivskyi" <report@bugs.python.org> wrote: > > Ivan Levkivskyi <levkivskyi@gmail.com> added the comment: > > > I'm not sure I understand the distinction. > > Initially I thought about only flagging code like this: > > @dataclass > class C: > x = field() > > But not this: > > @dataclass > class C: > x = 42 > > Now I think we should probably flag both as errors. > > > How do we only pick out `y` and probably `prop`, and ignore the rest, > without being overly fragile to new things being added? I guess ignoring > dunders and things in `__annotations__`. Is that close enough? > > We had a similar problem while developing Protocol class (PEP 544). > Currently we just a have a whitelist of names that are skipped: > > '__abstractmethods__', '__annotations__', '__weakref__', '__dict__', > '__slots__', '__doc__', '__module__' > > (plus some internal typing API names) > > ---------- > > _______________________________________ > Python tracker <report@bugs.python.org> > <https://bugs.python.org/issue32428> > _______________________________________ > |
|
|
| msg309416 - (view) |
Author: Ivan Levkivskyi (levkivskyi) * |
Date: 2018-01-03 16:49 |
| > I liked the original design better, where things without annotations would just be ignored. What changed? With the original proposal the ignored variables without annotations will behave as class variables. This "conflicts" with PEP 526 which requires class variables to be annotated with ClassVar[...]. On the other hand some people may be unhappy that they need to import `typing` to define a class variable in a dataclass. So this is a convenience vs consistence question. I am more in favour of consistence here, but only +0. |
|
|
| msg309417 - (view) |
Author: Ivan Levkivskyi (levkivskyi) * |
Date: 2018-01-03 16:52 |
| Just to clarify the previous comment, I still think that flagging this @dataclass class C: x = field() is important, since simply ignoring a ``field()`` will be too confusing (especially for ``attrs`` users). The previous comment is about @dataclass class C: x = 42 |
|
|
| msg309442 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2018-01-03 22:06 |
| > > I liked the original design better, where things without annotations would just be ignored. What changed? > With the original proposal the ignored variables without annotations will behave as class variables. This "conflicts" with PEP 526 which requires class variables to be annotated with ClassVar[...]. On the other hand some people may be unhappy that they need to import `typing` to define a class variable in a dataclass. So this is a convenience vs consistence question. I am more in favour of consistence here, but only +0. There is no real conflict with PEP 526 though. PEP 526 introduces ClassVar so the type checker can be made to understand. PEP 557 allows omitting ClassVar in case you don't care about type checkers. So I think we should stick with the current spec of PEP 557 (which lets you omit ClassVar), except for this special case: > I still think that flagging this > > @dataclass > class C: > x = field() > > is important, since simply ignoring a ``field()`` will be too confusing (especially for ``attrs`` users). Agreed. That's a special case and I'm fine with flagging it as an error. |
|
|
| msg309443 - (view) |
Author: Ivan Levkivskyi (levkivskyi) * |
Date: 2018-01-03 22:09 |
| > There is no real conflict with PEP 526 though. PEP 526 introduces ClassVar so the type checker can be made to understand. PEP 557 allows omitting ClassVar in case you don't care about type checkers. So I think we should stick with the current spec of PEP 557 (which lets you omit ClassVar), except for this special case: ... OK. |
|
|
| msg309584 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2018-01-06 22:19 |
| I'm closing this, and will open another issue to raise an error for: @dataclass class C: x = field() |
|
|
| msg309590 - (view) |
Author: Ivan Levkivskyi (levkivskyi) * |
Date: 2018-01-07 00:14 |
| @Eric > I'm closing this, and will open another issue to raise an error for: ... I think we also need a separate issue for not overriding __repr__ etc, if '__repr__' in cls.__dict__. |
|
|
| msg309591 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2018-01-07 00:17 |
| Correct. I'm working on that one now. I'll open it tonight or tomorrow. |
|
|