bpo-32214: Implement PEP 557: Data Classes by ericvsmith · Pull Request #4704 · python/cpython

ericvsmith · 2017-12-04T20:56:07Z

https://bugs.python.org/issue32214

serhiy-storchaka

I didn't follow the discussion about dataclasses, so there are mostly just style comments.

serhiy-storchaka · 2017-12-04T21:03:49Z

+        self._field_type = None
+
+    def __repr__(self):
+        return ('Field('


I think it would look better if add the f prefix to the first and the las chunks.

I'm not so concerned about this, and I'll probably rewrite the whole thing, anyway.

serhiy-storchaka · 2017-12-04T21:09:54Z

+
+    def __repr__(self):
+        return ('Field('
+                f'name={self.name!r},'


I think with spaces after commas it would look better.

serhiy-storchaka · 2017-12-04T21:12:41Z

+        return ('Field('
+                f'name={self.name!r},'
+                f'type={self.type},'
+                f'default={self.default},'


If default can be of arbitrary type, it is better to use a repr. In general case it is safer to use !r unless the value is a string and should formatted as a raw string. And this is faster.

serhiy-storchaka · 2017-12-04T21:17:21Z

+        self._field_type = None
+
+    def __repr__(self):
+        return ('Field('


I think the repr would look better if omit the optional parameters if they equal to the default value.

serhiy-storchaka · 2017-12-04T21:22:06Z

+    if len(fields) == 0:
+        return '()'
+    # Note the trailing comma, needed if this turns out to be a 1-tuple.
+    return f'({",".join([f"{obj_name}.{f.name}" for f in fields])},)'


This would look clearer if extract the internal expression as a variable. With space after comma the result would conform PEP 8. The trailing comma is needed only for a 1-tuple.

I'm not so concerned about this. It's generated code, and I don't particularly care how the generated code looks, as long as it's correct.

serhiy-storchaka · 2017-12-04T21:47:42Z

+
+        # Include InitVars and regular fields (so, not ClassVars).
+        _set_attribute(cls, '__init__',
+                       _init_fn(list(filter(lambda f: f._field_type


Could you please extract the lambda as a local function? This code is too complex.

I'll consider it. These expressions started out much simpler, and grew over time.

serhiy-storchaka · 2017-12-04T21:49:45Z

+
+    # Get the fields as a list, and include only real fields.  This is
+    #  used in all of the following methods.
+    field_list = list(filter(lambda f: f._field_type is _FIELD,


Wouldn't the code look clearer if use a list comprehension?

Probably so.

serhiy-storchaka · 2017-12-04T21:49:59Z

+
+    if repr:
+        _set_attribute(cls, '__repr__',
+                       _repr_fn(list(filter(lambda f: f.repr, field_list))))


The same as above.

serhiy-storchaka · 2017-12-04T21:50:57Z

+        generate_hash = hash
+    if generate_hash:
+        _set_attribute(cls, '__hash__',
+                       _hash_fn(list(filter(lambda f: f.compare


Could you please refactor this?

serhiy-storchaka · 2017-12-04T21:58:04Z

+            # Error if this field is specified in changes.
+            if f.name in changes:
+                raise ValueError(f'field {f.name} is declared with '
+                                 'init=False, it cannot be specified with '


ericvsmith · 2017-12-05T01:28:14Z

Thanks for the comments. I'll review the code with these in mind.

ericvsmith added 2 commits December 4, 2017 15:32

Implement PEP 557: Data Classes

9b883ca

Add blurb file.

96bf9b4

the-knights-who-say-ni added the CLA signed label Dec 4, 2017

bedevere-bot added the awaiting merge label Dec 4, 2017

ericvsmith changed the title ~~Implement PEP 557: Data Classes~~ bpo-32214 Implement PEP 557: Data Classes Dec 4, 2017

ericvsmith changed the title ~~bpo-32214 Implement PEP 557: Data Classes~~ bpo-32214: Implement PEP 557: Data Classes Dec 4, 2017

ericvsmith added 2 commits December 4, 2017 16:07

Fix whitespace.

0fa0778

Fix whitespace (again).

cfda03f

ericvsmith merged commit f0db54a into python:master Dec 4, 2017

bedevere-bot removed the awaiting merge label Dec 4, 2017

ericvsmith deleted the bpo-32214-data-classes branch December 4, 2017 21:59

serhiy-storchaka reviewed Dec 4, 2017

View reviewed changes

ericvsmith mentioned this pull request Dec 5, 2017

Misc questions, suggestions, and error corrections for dataclasses #4726

Closed

ericvsmith self-assigned this Dec 5, 2017

Uh oh!

Conversation

ericvsmith commented Dec 4, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka 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!

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!

ericvsmith commented Dec 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericvsmith commented Dec 4, 2017 •

edited by bedevere-bot

Loading