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 — bpo-42317: Improve docs of typing.get_args concerning Union by Dominik1123 · Pull Request #23254 · python/cpython

Conversation

@Dominik1123

@Dominik1123 Dominik1123 commented Nov 12, 2020 •
edited by miss-islington
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue42317

Automerge-Triggered-By: GH:gvanrossum

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dominik1123
bpo-42317: Improve docs of typing.get_args concerning Union
5a5ff4d
@Dominik1123
Dominik1123 requested review from gvanrossum and ilevkivskyi as code owners November 12, 2020 20:38
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 12, 2020
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Nov 12, 2020
gvanrossum
gvanrossum reviewed Nov 12, 2020
View reviewed changes
Comment thread Doc/library/typing.rst Outdated
For a typing object of the form ``X[Y, Z, ...]`` these functions return
``X`` and ``(Y, Z, ...)``. If ``X`` is a generic alias for a builtin or
:mod:`collections` class, it gets normalized to the original class.
If ``X`` is a :class:`Union`, the order of ``(Y, Z, ...)`` may be different

@gvanrossum gvanrossum Nov 12, 2020

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.

Suggested change
If ``X`` is a :class:`Union`, the order of ``(Y, Z, ...)`` may be different
If ``X`` is a :class:`Union` contained in another generic type, the order of ``(Y, Z, ...)`` may be different

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dominik1123 added 2 commits November 13, 2020 00:09
Dominik1123
Revert "bpo-42317: Improve docs of typing.get_args concerning Union"
adc974d 
This reverts commit 5a5ff4d.
Dominik1123
bpo-42317: Improve docs of typing.get_args concerning generic types
633e736
@Dominik1123
Dominik1123 requested a review from gvanrossum November 13, 2020 08:18
@Fidget-Spinner

Fidget-Spinner commented Nov 13, 2020

Copy link
Copy Markdown
Member

I don't really like the current proposed change because it's too broad, it may cause users to worry that all nested types are affected, when in reality it's only Union.

Instead of:

If X is a generic type, the returned objects (Y, Z, ...) might not be identical to the ones used in the form X[Y, Z, ...] due to type caching.

Guido's change seems a lot more apt:

If X is a :class:Union contained in another generic type, the order of (Y, Z, ...) may be different

A quick search through the typing code tells me that only the __hash__ and __eq__ methods of Union are order independent, no other typing type will suffer from this behavior when nested in another Generic.
All reactions

Sorry, something went wrong.

Uh oh!

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

Dominik1123 commented Nov 13, 2020

Copy link
Copy Markdown
Contributor Author

I don't really like the current proposed change because it's too broad, it may cause users to worry that all nested types are affected, when in reality it's only Union.

It's as broad as the promise of type caching goes, so I don't think it's too broad. But I agree that it's broader than necessary since indeed, at the moment, it requires a Union to be involved. Initially I thought it would also affect Literal, but apparently it doesn't:

>>> Literal[1, 2] == Literal[2, 1]
False

(Though I don't see why the two would be unequal; but that's a different concern)

Instead of:

If X is a generic type, the returned objects (Y, Z, ...) might not be identical to the ones used in the form X[Y, Z, ...] due to type caching.

Guido's change seems a lot more apt:

If X is a :class:Union contained in another generic type, the order of (Y, Z, ...) may be different

So I just revert back to Guido's version?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner

Fidget-Spinner commented Nov 13, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

(Though I don't see why the two would be unequal; but that's a different concern)

Wow good catch, that looks like it may be a bug.

So I just revert back to Guido's version?

Maybe a combination of both yours and Guido's wording:

If `X` is a :class:`Union` contained in another generic type, the order of `(Y, Z, ...)` may be different due to type caching.

Btw, thank you for the PR!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Dominik1123
bpo-42317: Improve docs of typing.get_args concerning Unions and gene…
e910d7f 
…ric types
Fidget-Spinner
Fidget-Spinner approved these changes Nov 13, 2020
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Nov 13, 2020
@gvanrossum gvanrossum added skip news needs backport to 3.9 labels Nov 16, 2020
gvanrossum
gvanrossum approved these changes Nov 16, 2020
View reviewed changes

@gvanrossum gvanrossum 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, LG!

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 core review labels Nov 16, 2020
@gvanrossum gvanrossum added the 🤖 automerge label Nov 16, 2020
@miss-islington
miss-islington merged commit c3b9592 into python:master Nov 16, 2020
@miss-islington

miss-islington commented Nov 16, 2020

Copy link
Copy Markdown
Contributor

Thanks @Dominik1123 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 16, 2020
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 16, 2020
@Dominik1123 @miss-islington
bpo-42317: Improve docs of typing.get_args concerning Union (pythonGH…
4edf578 
…-23254)

(cherry picked from commit c3b9592)

Co-authored-by: Dominik1123 <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Nov 16, 2020
@bedevere-bot

bedevere-bot commented Nov 16, 2020

Copy link
Copy Markdown

GH-23307 is a backport of this pull request to the 3.9 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Nov 16, 2020
@miss-islington @Dominik1123
bpo-42317: Improve docs of typing.get_args concerning Union (GH-23254)
2369759 
(cherry picked from commit c3b9592)

Co-authored-by: Dominik1123 <[email protected]>
@Dominik1123 Dominik1123 mentioned this pull request Nov 16, 2020
bpo-42345: Fix three issues with typing.Literal parameters #23294
Merged
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@Dominik1123 @adorilson
bpo-42317: Improve docs of typing.get_args concerning Union (pythonGH…
6e0be5b 
…-23254)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@ilevkivskyi ilevkivskyi Awaiting requested review from ilevkivskyi

Uh oh!

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

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

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.

6 participants

@Dominik1123 @Fidget-Spinner @miss-islington @bedevere-bot @gvanrossum @the-knights-who-say-ni
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.