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

gh-102615: Use list instead of tuple in repr of paramspec #102637

Merged
merged 5 commits into from Mar 15, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Mar 13, 2023

@Fidget-Spinner
Copy link
Member

I'm -1 on this fix. There is a bigger underlying issue. See #102615 (comment)

@gvanrossum
Copy link
Member

I have no idea what to do here, I defer to @Fidget-Spinner.

@Fidget-Spinner
Copy link
Member

I'm now +1 on this approach after Nikita reminded me that ParamSpec intentionally has edge cases that we cant fix.

@sobolevn
Copy link
Member Author

This PR should be reviewed after #102681
I think I might need to modify it afterwards :)

@sobolevn sobolevn marked this pull request as draft March 14, 2023 07:20
@sobolevn sobolevn marked this pull request as ready for review March 14, 2023 14:41
@sobolevn
Copy link
Member Author

@AlexWaygood you can review this whenever you have the time.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

@AlexWaygood you can review this whenever you have the time.

Time, the most precious commodity of our age! Here's a review for the tests and docs :)

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood AlexWaygood self-requested a review March 14, 2023 15:11
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@gvanrossum gvanrossum removed their request for review March 14, 2023 17:21
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, at least as a short-term fix.

I don't think we should backport this, as it could easily break people's doctests. Let me know if you disagree :)

TsP[int, str, list[int], []]: "TsP[int, str, list[int], []]",
TsP[int, [str, list[int]]]: "TsP[int, [str, list[int]]]",

# These lines are just too long to fit:
Copy link
Member

Choose a reason for hiding this comment

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

😆

@AlexWaygood AlexWaygood merged commit 2b5781d into python:main Mar 15, 2023
12 checks passed
@AlexWaygood
Copy link
Member

@sobolevn, should we make the same changes we made here to _collections_abc._type_repr? The docstring of that function indicates it is meant to have identical functionality to typing._type_repr.

def _type_repr(obj):

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 16, 2023

In fact... I wonder if we should just import _type_repr from _collections_abc in typing, instead of duplicating it between the two modules. It's obvious why _collections_abc shouldn't depend on typing, but I don't see any reason why typing shouldn't depend on _collections_abc. We already have import collections.abc at the top of the file.

@sobolevn
Copy link
Member Author

sobolevn commented Mar 16, 2023

@AlexWaygood I thought about making the similar change in _collections_abc._type_repr. But, what is the use-case?

Right now users can define their own classes with ParamSpec only using typing.Generic. So, no use for _collections_abc.
As far as I know, the only std type that supports it is _collections_abc.Callable, but it already supports the correct repr.

Any examples where this might be useful?

I also think that sharing implementation details (functions starting with _) creates high coupling and implicit contracts, which is bad thing.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 16, 2023

Okay, those are all good points. I think it might be nice at least to update the docstring of typing._type_repr to remind us that there's a function with very similar functionality in _collections_abc, though. What do you think?

@sobolevn
Copy link
Member Author

I will!

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
…python#102637)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
…python#102637)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
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

5 participants