GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-102615: Use `list` instead of `tuple` in `repr` of paramspec by sobolevn · Pull Request #102637 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Mar 13, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Representation of ParamSpecs at runtime compared to Callable #102615

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Gobot1234 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@sobolevn
pythongh-102615: Use list instead of tuple in repr of paramspec
8e4620c
@sobolevn
sobolevn requested review from AlexWaygood, Fidget-Spinner, JelleZijlstra and gvanrossum as code owners March 13, 2023 09:07
@bedevere-bot bedevere-bot mentioned this pull request Mar 13, 2023
Representation of ParamSpecs at runtime compared to Callable #102615
Closed
@bedevere-bot bedevere-bot added the awaiting review label Mar 13, 2023
@Fidget-Spinner

Fidget-Spinner commented Mar 13, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood AlexWaygood added the DO-NOT-MERGE label Mar 13, 2023
@gvanrossum

gvanrossum commented Mar 13, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner Fidget-Spinner removed the DO-NOT-MERGE label Mar 14, 2023
@Fidget-Spinner

Fidget-Spinner commented Mar 14, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn sobolevn mentioned this pull request Mar 14, 2023
gh-102615: Fix type vars substitution of collections.abc.Callable and custom generics with ParamSpec #102681
Closed
@sobolevn

sobolevn commented Mar 14, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

sobolevn commented Mar 14, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood reviewed Mar 14, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Misc/NEWS.d/next/Library/2023-03-13-12-05-55.gh-issue-102615.NcA_ZL.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn @AlexWaygood
Apply suggestions from code review
4e12bd6
Co-authored-by: Alex Waygood <[email protected]>
@AlexWaygood
AlexWaygood self-requested a review March 14, 2023 15:11
AlexWaygood
AlexWaygood reviewed Mar 14, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn @AlexWaygood
Update Lib/test/test_typing.py
d226833
Co-authored-by: Alex Waygood <[email protected]>
AlexWaygood
AlexWaygood reviewed Mar 14, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn @AlexWaygood
Update Lib/test/test_typing.py
dc36254
Co-authored-by: Alex Waygood <[email protected]>
@gvanrossum
gvanrossum removed their request for review March 14, 2023 17:21
AlexWaygood
AlexWaygood reviewed Mar 14, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood reviewed Mar 14, 2023
View reviewed changes
Comment thread Lib/typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood reviewed Mar 14, 2023
View reviewed changes
Comment thread Lib/typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn
Address review
3451d4d
AlexWaygood
AlexWaygood approved these changes Mar 15, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_typing.py
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:

@AlexWaygood AlexWaygood Mar 15, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

😆

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Mar 15, 2023
@AlexWaygood
AlexWaygood merged commit 2b5781d into python:main Mar 15, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 15, 2023
@AlexWaygood AlexWaygood mentioned this pull request Mar 16, 2023
gh-102721: Improve coverage of _collections_abc._CallableGenericAlias #102722
Merged
@AlexWaygood

AlexWaygood commented Mar 16, 2023

Copy link
Copy Markdown
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.

cpython/Lib/_collections_abc.py

Line 515 in 51d693c

def _type_repr(obj):

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Mar 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Mar 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Mar 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Mar 16, 2023

Copy link
Copy Markdown
Member Author

I will!

AlexWaygood reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn sobolevn mentioned this pull request Mar 16, 2023
Add comments to {typing,_collections_abc}._type_repr about each other #102752
Merged
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@sobolevn @AlexWaygood @Fidget-Spinner
pythongh-102615: Use list instead of tuple in repr of paramspec (…
4de1e65
…python#102637)

Co-authored-by: Alex Waygood <[email protected]>
@Gobot1234 Gobot1234 mentioned this pull request Apr 9, 2023
Fix old behaviour in typing documentation #103400
Merged
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@sobolevn @AlexWaygood @warsaw
pythongh-102615: Use list instead of tuple in repr of paramspec (…
e5a5a95
…python#102637)

Co-authored-by: Alex Waygood <[email protected]>
@sobolevn sobolevn mentioned this pull request Jun 8, 2023
Inconsistent repr of types.GenericAlias with ParamSpec #105486
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@sobolevn @Fidget-Spinner @gvanrossum @AlexWaygood @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.