-
-
Notifications
You must be signed in to change notification settings - Fork 117
Concatenate to support Ellipsis argument #481
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
Merged
JelleZijlstra
merged 23 commits into
python:main
from
Daraan:concatenate/ellipsis-support-110
Oct 22, 2024
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
7e0aac8
Backport Ellipsis in Concatenate for 3.9-3.10
Daraan 73d3952
Extended ellipsis test, removed version_info slices
Daraan 2e5fc74
Merge branch 'main' into main
Daraan c716ff1
Merge 'upstream/main' into concatenate/ellipsis
Daraan 70d5205
Properly skipped tests
Daraan 8e4f0be
renamed function better reasoning
Daraan 0991b42
Properly skipped tests
Daraan de6a2c6
Merge remote-tracking branch 'upstream/main' into concatenate/ellipsis
Daraan 3fb63a3
Merge remote-tracking branch 'upstream/main' into concatenate/ellipsis
Daraan 3112a80
Added info to changelog and docs
Daraan 1eabcb6
Full backport to 3.8 to support Concatenate[...]
Daraan 3e6ec0a
Updated changelog and docs
Daraan 5b9e4cb
Merge branch 'main' into main
Daraan a77d93f
minor comment and name update
Daraan 8cba5ab
Changed invalid use example to current behavior
Daraan 65270c6
Changed ref to new PR
Daraan 97bdfd1
Merge branch 'main' into concatenate/ellipsis-support-110
Daraan ff79863
Merge remote-tracking branch 'upstream/main' into concatenate/ellipsi…
Daraan 9fda1a1
Unified typing.Callable and collections.abc.Callable tests
Daraan c092d7e
fix whitespace
Daraan 4e8ef02
Merge remote-tracking branch 'upstream/main' into concatenate/ellipsi…
Daraan c5229f8
Minor changes
Daraan c06a9c7
unified invalid tests and additional test case
Daraan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
If you make this inherit from ParamSpec, can it fix 3.10.3+?
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.
I might be missing something here. To what case are you referring here?
This backport of
Concatenate[...]
also works for all 3.10, as the critical part is in_concatenate_getitem
Is it possibly related to the following test I jointly introduced also in #479?
That aside; It is possible to do
_EllipsisDummy = ParamSpec("_EllipsisDummy")
if needed as subclassingParamSpec
is (currently) not possible. I thought using a new class might have less side-effects than using a ParamSpec instance.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.
I'm referring to the comment right above here, about the test that works only in 3.10.0-3.10.2 but not 3.10.3+. Making
_EllipsisDummy
an instance of ParamSpec might fix that test for later versions of 3.10.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.
I am sorry for the confusion. I do think we talk about the same location :), if not correct me please. I made the necessary edits above.
#479 allows to backport that test to all versions of 3.10, however for #479 the test
self.assertEqual(get_args(C1), (Concatenate[int, ...], Any))
would need this PR to work without an error on 3.10 as well.Both PRs are needed make this test work for the 3.10 version range.
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.
I see, thanks. #479 is merged now, so would you mind updating this PR? There's a merge conflict right now.
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.
Sure. Unified the code and the test we talked about, aside from that added one more test for invalid usage.