Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make NamedTuple provide __new__ instead of __init__ #5643

Merged
merged 3 commits into from
Sep 19, 2018
Merged

Conversation

msullivan
Copy link
Collaborator

Closes #1279.

Depends on either #5642 or a typeshed sync

@msullivan msullivan requested review from JukkaL, ilevkivskyi and gvanrossum and removed request for JukkaL September 19, 2018 18:50
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question about new error message that may be confusing.

@@ -587,7 +587,7 @@ class XMethBad(NamedTuple):
class MagicalFields(NamedTuple):
x: int
def __slots__(self) -> None: pass # E: Cannot overwrite NamedTuple attribute "__slots__"
def __new__(cls) -> None: pass # E: Cannot overwrite NamedTuple attribute "__new__"
def __new__(cls) -> None: pass # E: Name '__new__' already defined on line 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is not perfect but probably OK. Why don't you want to keep the old error message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the old error message TBH but the interaction between the namedtuple override checks and the method actually existing seemed nontrivial and I didn't think it was worth fighting with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is hard, then I think it is OK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it a bit and it turns out to be very easy to fix as part of another PR I am putting together. I'm going to land this now with the error message regression (to avoid needing to stack this on top of the other PR, which might require more back-and-forth) and then fix the error message in that PR.

@msullivan msullivan changed the base branch from new_over_init to master September 19, 2018 21:43
@msullivan msullivan merged commit a88afb2 into master Sep 19, 2018
@msullivan msullivan deleted the namedtuple-new branch September 19, 2018 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants