Skip to content

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

Closed
rhettinger wants to merge 3 commits into
python:masterfrom
rhettinger:dataclass_mods
Closed

Misc questions, suggestions, and error corrections for dataclasses#4726
rhettinger wants to merge 3 commits into
python:masterfrom
rhettinger:dataclass_mods

Conversation

@rhettinger
Copy link
Copy Markdown
Contributor

@rhettinger rhettinger commented Dec 5, 2017 •
edited
Loading

These are for Eric's consideration:

  • Fix typo leading to a SyntaxError in the make_dataclass docstring
  • Let make_dataclass pass through keyword arguments (ordered, hash, frozen, etc)
  • Replace "len(s)==0" with the faster and more idiomatic (PEP 8ish) "not s"
  • Ask whether asdict() should return an OrderedDict instead of a regular dict
  • Even though the class implementation details have "hash=None" as a way of turning off hashing, the dataclass API should use regular booleans and have the default be "hash=False" (as equivalent to "hash=None"). It is a bit of an API landmine to have hashing succeed in this example:
>>> from dataclasses import dataclass
>>> @dataclass(hash=False)
... class C:
...     x: int
...
>>> hash(C(999))
-9223372036585142703

@rhettinger rhettinger requested a review from ericvsmith December 5, 2017 06:17
@ericvsmith
Copy link
Copy Markdown
Member

ericvsmith commented Dec 5, 2017

Thanks, @rhettinger. @serhiy-storchaka had many of these suggestions over at #4704, and I've been working on a patch for his issues. I'll answer your questions when I have a few minutes to devote to it, but it will probably have to wait until the weekend.

@ericvsmith
Copy link
Copy Markdown
Member

ericvsmith commented Jan 21, 2018

Of the original issues, I think most have been addressed in subsequent commits (many not by me).

The remaining issues are:

  • use "if fields" instead of "if len(fields) == 0"
  • an issue with hashing.

I'm still investigating all of the hashing implications, and how it interacts with bpo-32513.

For the "if fields" issue, I'll just go ahead and commit that when I'm making other changes to dataclasses. So, I think this PR can be closed. I'll close it once I'm done with the hashing questions.

@rhettinger rhettinger closed this Dec 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericvsmith ericvsmith Awaiting requested review from ericvsmith

Assignees

@ericvsmith ericvsmith

Labels

awaiting merge skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rhettinger @ericvsmith @the-knights-who-say-ni @bedevere-bot