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

Add support for namedtuple methods (issue #1076) #1810

Merged
merged 85 commits into from Sep 2, 2016

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Jul 6, 2016

Added NamedTupleType

Caveats:

  • Subclasses cannot add mutable fields
  • Can't call _make on an instance


X = namedtuple('X', ['x', 'y'])
x = None # type: X
a = x._asdict()
Copy link
Member

Choose a reason for hiding this comment

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

Reveal the type of a.

Copy link
Contributor Author

@elazarg elazarg Jul 6, 2016

Choose a reason for hiding this comment

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

As with _replace (and as documented in the code), I don't know how to return OrderedDict[str, Union[types]], so it is untested.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@elazarg elazarg Jul 6, 2016

Choose a reason for hiding this comment

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

I am not familiar enough with the code to estimate which approach would be better. Merging with the template might work but feels hackish to me (like the existing approach); the current approach was intended to be minimal, and does not make it any harder to merge with the template later.

I believe that the "right" approach would be to to make NameTuple a first-class type constructor, like class is. But I think that it requires a much deeper surgery.

Where are the object methods coming from? It is a similar question.

Copy link
Member

Choose a reason for hiding this comment

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

We cannot promote NamedTuple to the same level as class -- that would require changes to the Python syntax. (There's also some history where the original author of namedtuple resists changes.)

For most built-in types (including object) the definition in builtins is read by mypy, and the additional behavior implemented by mypy is done subtly by checking in various cases whether a type's full name is e.g. builtins.object.

NamedTuple/namedtuple are treated differently because it is essential that mypy knows the names of the fields (and their types, in the case of NamedTuple). But I think merging this behavior with a definition in collections or collections.abc is the most robust approach here.

Shall I close this PR or are you just going to try it on top of this one?

Copy link
Contributor Author

@elazarg elazarg Jul 7, 2016

Choose a reason for hiding this comment

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

Let me have a try. I might still need help in referencing type(self)
correctly; I am not sure that a template will help.

Copy link
Member

Choose a reason for hiding this comment

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

Great! Feel free to ask questions in this PR. To direct the work I recommend making up some test cases and then tracing through the code with pdb or PyCharm's debugger -- that's often how I do things. The README.md has some hints on how to run a specific test case using myunit, without the overhead of runtests.py.

X = NamedTuple('X', [('x', int), ('y', str)])
a = None # type: str
b = None # type: str
a, b = X._fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use reveal_type.

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 31, 2016

Thanks for simplifying the code! Other than the comments above, looks good now! And one more thing: there's no test case for _field_types.

@elazarg
Copy link
Contributor Author

elazarg commented Sep 1, 2016

typeshed/#518: Add TypeAlias for OrderedDict is needed for the tests to pass.


class OrderedDict:
class OrderedDict(Generic[KT, KV]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to be used anymore?

reveal_type(X._make([5, 'a'])) # E: Revealed type is 'Tuple[builtins.int, builtins.str, fallback=__main__.X]'
X._make('a b') # E: Argument 1 to X._make has incompatible type "str"; expected Iterable[Any]

-- # FIX: not a proper class method
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once this has landed, can you create a new issue for the most important todo items?

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 2, 2016

Looks good now! 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.

None yet

4 participants