Skip to content

Support namedtuple as one of multiple superclasses #2091

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

Merged
merged 6 commits into from
Sep 8, 2016

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Sep 3, 2016

Solve issue #1558 by delegate subtype checks on TupleType to fallback.

This does not solve #1485. I don't know why.

@elazarg elazarg changed the title Support namedtuple as one of multiple superclasses (issue #1558) Support namedtuple as one of multiple superclasses Sep 3, 2016
@JukkaL
Copy link
Collaborator

JukkaL commented Sep 7, 2016

Looks good to me, though there is one potential simplification. The if is_named_instance(right, 'builtins.object') check above your change now seems unnecessary, and maybe also the Sized check. Can you see if you can remove them to simplify the code a bit?

@elazarg
Copy link
Contributor Author

elazarg commented Sep 7, 2016

builtins.object can be omitted. Sized cannot.

@elazarg
Copy link
Contributor Author

elazarg commented Sep 7, 2016

Yes - more refactoring. It helped me understand what happens, and I believe it makes the logic clearer.

I can revert back to the minimal change if necessary.

@elazarg
Copy link
Contributor Author

elazarg commented Sep 7, 2016

In the experimental refactor_to_enum branch I encountered what seems like another case of "Instance leak" we've discussed some time ago. I will try to investigate it; should I add the fix here or in a different PR?

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 7, 2016

If it seems unrelated to this change, it's probably better create a new PR.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 7, 2016

The new methods don't really make the code easier for me to read. I'd suggest reverting to having them inline instead, as they are sufficiently simple.

@elazarg
Copy link
Contributor Author

elazarg commented Sep 7, 2016

I would have opened an issue for the error, but it's hard to reproduce; in that branch there are many occurrences of the type, with similar context, and the error only occur in one of them.

The current patch fixes the error, but I don't think it addresses the root of the problem.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 8, 2016

Okay if it's hard to isolate then no problem.

@JukkaL JukkaL merged commit 590c15f into python:master Sep 8, 2016
@JukkaL
Copy link
Collaborator

JukkaL commented Sep 8, 2016

Thanks!

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