bpo-32214: Implement PEP 557: Data Classes #4704
Conversation
ericvsmith
changed the title
ericvsmith
changed the title
| self._field_type = None | ||
|
|
||
| def __repr__(self): | ||
| return ('Field(' |
There was a problem hiding this comment.
I think it would look better if add the f prefix to the first and the las chunks.
There was a problem hiding this comment.
I'm not so concerned about this, and I'll probably rewrite the whole thing, anyway.
|
|
||
| def __repr__(self): | ||
| return ('Field(' | ||
| f'name={self.name!r},' |
There was a problem hiding this comment.
I think with spaces after commas it would look better.
| return ('Field(' | ||
| f'name={self.name!r},' | ||
| f'type={self.type},' | ||
| f'default={self.default},' |
There was a problem hiding this comment.
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.
| self._field_type = None | ||
|
|
||
| def __repr__(self): | ||
| return ('Field(' |
There was a problem hiding this comment.
I think the repr would look better if omit the optional parameters if they equal to the default value.
| 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])},)' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| # Include InitVars and regular fields (so, not ClassVars). | ||
| _set_attribute(cls, '__init__', | ||
| _init_fn(list(filter(lambda f: f._field_type |
There was a problem hiding this comment.
Could you please extract the lambda as a local function? This code is too complex.
There was a problem hiding this comment.
I'll consider it. These expressions started out much simpler, and grew over time.
|
|
||
| # 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, |
There was a problem hiding this comment.
Wouldn't the code look clearer if use a list comprehension?
|
|
||
| if repr: | ||
| _set_attribute(cls, '__repr__', | ||
| _repr_fn(list(filter(lambda f: f.repr, field_list)))) |
| generate_hash = hash | ||
| if generate_hash: | ||
| _set_attribute(cls, '__hash__', | ||
| _hash_fn(list(filter(lambda f: f.compare |
There was a problem hiding this comment.
Could you please refactor this?
| # 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 ' |
|
Thanks for the comments. I'll review the code with these in mind. |
https://bugs.python.org/issue32214