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

Handling of field names tuple (#4406) #4407

Merged
merged 1 commit into from Jan 12, 2018

Conversation

Projects
None yet
3 participants
@hoefling
Contributor

hoefling commented Dec 24, 2017

Is this fix for #4406 acceptable? My concern is that in this solution, the tuple case is merely a duplication of the list case, with only the output formatting being different.

@elazarg

I don't think the code needs to be duplicated. You can just check isinstance(rvalue.args[1], (ListExpr, TupleExpr))

@elazarg

This comment has been minimized.

Contributor

elazarg commented Dec 24, 2017

I don't think the formatting is important.

@hoefling

This comment has been minimized.

Contributor

hoefling commented Dec 24, 2017

@elazarg: I thought about that too, but then I'm unsure about getting the proper formatting for the field names (['spam', 'eggs'] vs. ('spam', 'eggs')). Wouldn't I need to check the exact type again for that? Smth like this:

elif isinstance(rvalue.args[1], (ListExpr, TupleExpr)):
    list_items = cast(List[StrExpr], rvalue.args[1].items)
    format = '[%s]' if isinstance(rvalue.args[1], ListExpr) else '(%s)'
    items = format % ', '.join(repr(item.value) for item in list_items)
@elazarg

This comment has been minimized.

Contributor

elazarg commented Dec 24, 2017

Why use different formatting?

@hoefling

This comment has been minimized.

Contributor

hoefling commented Dec 24, 2017

Because otherwise the stub would lie about the actual code? I mean, there must be a reason why a distinction between the list and string case was implemented the way it is now; one could just use the same repr for both cases, but it's different. I don't want to ignore it without knowing what's the intention behind it.

@elazarg

This comment has been minimized.

Contributor

elazarg commented Dec 24, 2017

Well I think it's the same code with different syntax - just like difference in whitespace - but it's not very important either way.

@gvanrossum

Also please add a test or two to test-data/unit/stubgen.test.

@@ -596,6 +596,9 @@ def process_namedtuple(self, lvalue: NameExpr, rvalue: CallExpr) -> None:
elif isinstance(rvalue.args[1], ListExpr):
list_items = cast(List[StrExpr], rvalue.args[1].items)
items = '[%s]' % ', '.join(repr(item.value) for item in list_items)
elif isinstance(rvalue.args[1], TupleExpr):
list_items = cast(List[StrExpr], rvalue.args[1].items)
items = '(%s)' % ', '.join(repr(item.value) for item in list_items)

This comment has been minimized.

@gvanrossum

gvanrossum Dec 24, 2017

Member

This would actually be broken if the original tuple had only one item. Reusing the previous stanza would avoid that.

This comment has been minimized.

@hoefling

hoefling Dec 24, 2017

Contributor

@gvanrossum oh, now I see - without the trailing comma, it's a generator expression...

@hoefling

This comment has been minimized.

Contributor

hoefling commented Dec 25, 2017

Changes: Using the suggestion @elazarg has made to avoid the error; added a unit test that validates cases where fields were passed as a tuple with no elements/one element/multiple elements. Try the added test out by running

$ pytest -v -n1 mypy/test/teststubgen.py::StubgenPythonSuite::testNamedtupleAltSyntaxFieldsTuples
@elazarg

This comment has been minimized.

Contributor

elazarg commented Dec 25, 2017

(You probably want -n0 - no parallelism at all)

@hoefling

This comment has been minimized.

Contributor

hoefling commented Dec 25, 2017

Oh, this indeed makes much more sense - thanks! My learning curve for pytest and its plugins is still pretty steep...

@hoefling hoefling changed the title from added handling of field names tuple (#4406) to Handling of field names tuple (#4406) Dec 28, 2017

@hoefling

This comment has been minimized.

Contributor

hoefling commented Jan 12, 2018

@gvanrossum: @elazarg: is the PR now mature enough to be accepted? Sorry if I'm being annoying, this is my first time doing a PR so I don't know if I did everything right...

@elazarg

This comment has been minimized.

Contributor

elazarg commented Jan 12, 2018

LGTM, but I am not familiar enough with stubgen.

@gvanrossum gvanrossum merged commit 468dff2 into python:master Jan 12, 2018

2 checks passed

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

This comment has been minimized.

Member

gvanrossum commented Jan 12, 2018

Thanks!

@hoefling hoefling deleted the hoefling:4406 branch Jan 12, 2018

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