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

Support additional args to namedtuple() #5215

Merged
merged 11 commits into from Aug 3, 2018

Conversation

Projects
None yet
3 participants
@JelleZijlstra
Collaborator

JelleZijlstra commented Jun 14, 2018

By letting typeshed do the work.

Fixes #2124, #2127, #4788.

@@ -1,7 +1,7 @@
[case testNamedTupleUsedAsTuple]
from collections import namedtuple
X = namedtuple('X', ['x', 'y'])
X = namedtuple('X', 'x y')

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

My change ends up requiring list to exist in all of these tests. It was easier to just change the namedtuple call than to add [builtins fixtures/list.pyi] everywhere. I left a few with the list to make sure we test that code path.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

My change ends up requiring list to exist in all of these tests. It was easier to just change the namedtuple call than to add [builtins fixtures/list.pyi] everywhere. I left a few with the list to make sure we test that code path.

Show outdated Hide outdated test-data/unit/check-namedtuple.test Outdated
@@ -13,14 +13,6 @@ s = b'foo'
from typing import TypeVar
T = TypeVar(u'T')
[case testNamedTupleUnicode]

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

This no longer worked because my stub for namedtuple just has str. Apparently we don't support Text in the test stubs, so I just moved this test case to the pythoneval tests.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

This no longer worked because my stub for namedtuple just has str. Apparently we don't support Text in the test stubs, so I just moved this test case to the pythoneval tests.

@@ -27,3 +27,4 @@ class int:
class str: pass
class bool: pass
class function: pass
class ellipsis: pass

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

This was needed because a few tests that import collections didn't have ... available in their fixtures, and my stub for namedtuple uses it.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

This was needed because a few tests that import collections didn't have ... available in their fixtures, and my stub for namedtuple uses it.

@@ -138,10 +138,6 @@ MypyFile:1(
from collections import namedtuple
N = namedtuple('N') # E: Too few arguments for namedtuple()
[case testNamedTupleWithTooManyArguments]

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

This test stopped working because this error is now caught during type checking rather than semanal.

@JelleZijlstra

JelleZijlstra Jun 14, 2018

Collaborator

This test stopped working because this error is now caught during type checking rather than semanal.

JelleZijlstra added some commits Jun 14, 2018

JelleZijlstra added some commits Jun 14, 2018

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Aug 2, 2018

Member

Did anyone else review this yet? It seems oddly stalled (I wonder what happened around June 13, a number of PRs saw their last activity around that date).

Member

gvanrossum commented Aug 2, 2018

Did anyone else review this yet? It seems oddly stalled (I wonder what happened around June 13, a number of PRs saw their last activity around that date).

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Aug 2, 2018

Collaborator

No. As far as I can see, it's ready for review.

Collaborator

JelleZijlstra commented Aug 2, 2018

No. As far as I can see, it's ready for review.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 2, 2018

Collaborator

I can review it later today.

Collaborator

ilevkivskyi commented Aug 2, 2018

I can review it later today.

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Aug 2, 2018

Collaborator
Collaborator

JelleZijlstra commented Aug 2, 2018

@ilevkivskyi

Thanks for the fixes! Here are few comments.

Show outdated Hide outdated mypy/checkexpr.py Outdated
Show outdated Hide outdated mypy/semanal_namedtuple.py Outdated
Show outdated Hide outdated mypy/semanal_namedtuple.py Outdated
Show outdated Hide outdated test-data/unit/check-namedtuple.test Outdated
Show outdated Hide outdated test-data/unit/lib-stub/collections.pyi Outdated
@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Aug 3, 2018

Collaborator

Thanks for the review! I will push an update soon.

Collaborator

JelleZijlstra commented Aug 3, 2018

Thanks for the review! I will push an update soon.

JelleZijlstra added some commits Aug 3, 2018

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Aug 3, 2018

Collaborator

Will fix the CI failure later today.

Collaborator

JelleZijlstra commented Aug 3, 2018

Will fix the CI failure later today.

@ilevkivskyi

Thanks! One last comment. Could you please also fix the self-check? You can merge it after all tests pass.

Show outdated Hide outdated mypy/semanal_namedtuple.py Outdated

@JelleZijlstra JelleZijlstra merged commit 223d104 into python:master Aug 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:ntdefaults branch Aug 3, 2018

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Aug 3, 2018

Member

So the initial description said "Fixes #2124, #2127, #4788." And only #2124 got auto-closed (auto-close isn't smart enough :-). Should the other two also be closed now?

Member

gvanrossum commented Aug 3, 2018

So the initial description said "Fixes #2124, #2127, #4788." And only #2124 got auto-closed (auto-close isn't smart enough :-). Should the other two also be closed now?

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Aug 3, 2018

Collaborator

Thanks for catching that! I'll close the other ones too.

Collaborator

JelleZijlstra commented Aug 3, 2018

Thanks for catching that! I'll close the other ones too.

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Aug 3, 2018

Collaborator

I suppose I should have written "Fixes #2124. Fixes #2127. Fixes #4788."

Collaborator

JelleZijlstra commented Aug 3, 2018

I suppose I should have written "Fixes #2124. Fixes #2127. Fixes #4788."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment