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

fix: removing typing and duplicate class_ for KeysView/ValuesView/ItemsView. #4985

Merged
merged 2 commits into from
Jan 13, 2024

Conversation

hczhai
Copy link
Contributor

@hczhai hczhai commented Dec 20, 2023

Description

Another alternative to PR #4972 and PR #4983. Fixes issue #4529.
See #4983 (comment)

Suggested changelog entry:

Remove typing and duplicate ``class_`` for ``KeysView``/``ValuesView``/``ItemsView``

@rwgk
Copy link
Collaborator

rwgk commented Dec 20, 2023

Thanks a lot @hczhai!

I'll try global testing with this PR, too.

@rwgk
Copy link
Collaborator

rwgk commented Dec 21, 2023

I'll try global testing with this PR, too.

I succeeded only now to initiate the global testing for this PR (after two failed attempts due to unrelated issues), it'll be another 8 hours or so before I have the results.

Assuming the global testing passes (seems very likely to me), I think merging this PR is our best alternative. It's better than the state before #4353 (reduced code complexity and runtime overhead, sidesteps #3986) and fixes #4529.

@hczhai what do you think about transferring a few selected tests from #4972 and #4983 here? This is in case someone wants to add the [K, V] etc. annotations later, to make it less likely that they accidentally reintroduce issues for use cases we hit on already. Specifically, the long long int, str, and maybe object use cases.

@rwgk
Copy link
Collaborator

rwgk commented Dec 21, 2023

Global testing passed, with diffs in only one stubgen test again (same test as before). (Internal test id for my own reference: OCL:592563041:BASE:592858163:1703173245690:a9e29695)

The diff is as expected:

-class ItemsView[int, object]:
+class ItemsView:
     def __init__(self, *args, **kwargs) -> None: ...
     def __iter__(self) -> Iterator: ...
     def __len__(self) -> int: ...

-class ItemsView[str, object]:
+class KeysView:
     def __init__(self, *args, **kwargs) -> None: ...
-    def __iter__(self) -> Iterator: ...
-    def __len__(self) -> int: ...
-
-class KeysView[int]:
-    def __init__(self, *args, **kwargs) -> None: ...
-    @overload
-    def __contains__(self, arg0: int) -> bool: ...
-    @overload
-    def __contains__(self, arg0: object) -> bool: ...
-    def __iter__(self) -> Iterator: ...
-    def __len__(self) -> int: ...
-
-class KeysView[str]:
-    def __init__(self, *args, **kwargs) -> None: ...
-    @overload
-    def __contains__(self, arg0: str) -> bool: ...
-    @overload
     def __contains__(self, arg0: object) -> bool: ...
     def __iter__(self) -> Iterator: ...
     def __len__(self) -> int: ...

 class OpsById:
     def __init__(self) -> None: ...
-    def items(self) -> ItemsView[int,object]: ...
-    def keys(self) -> KeysView[int]: ...
-    def values(self) -> ValuesView[object]: ...
+    def items(self) -> ItemsView: ...
+    def keys(self) -> KeysView: ...
+    def values(self) -> ValuesView: ...
     def __bool__(self) -> bool: ...
     @overload
     def __contains__(self, arg0: int) -> bool: ...
@@ -101,9 +84,9 @@

 class OpsByName:
     def __init__(self) -> None: ...
-    def items(self) -> ItemsView[str,object]: ...
-    def keys(self) -> KeysView[str]: ...
-    def values(self) -> ValuesView[object]: ...
+    def items(self) -> ItemsView: ...
+    def keys(self) -> KeysView: ...
+    def values(self) -> ValuesView: ...
     def __bool__(self) -> bool: ...
     @overload
     def __contains__(self, arg0: str) -> bool: ...
@@ -331,7 +314,7 @@
 class TF_Status:
     def __init__(self, *args, **kwargs) -> None: ...

-class ValuesView[object]:
+class ValuesView:
     def __init__(self, *args, **kwargs) -> None: ...
     def __iter__(self) -> Iterator: ...
     def __len__(self) -> int: ...

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@aimir what's your opinion?

@rwgk
Copy link
Collaborator

rwgk commented Jan 9, 2024

@aimir @sizmailov I'm still interested in your review of this PR. I'll wait another couple days.

@rwgk
Copy link
Collaborator

rwgk commented Jan 13, 2024

There was no feedback from anyone for 3 weeks: merging now to fix #4529, which was confirmed by @aimir to be a "significant issue":

This is a significant issue, and fixing this sooner rather than later is more important than having nice typing hints.

@rwgk rwgk merged commit 31b7e14 into pybind:master Jan 13, 2024
84 checks passed
Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

LGTM

@henryiii henryiii added the needs changelog Possibly needs a changelog entry label Mar 27, 2024
@henryiii henryiii changed the title bugfix: removing typing and duplicate class_ for KeysView/ValuesView/ItemsView. Fix #4529 fix: removing typing and duplicate class_ for KeysView/ValuesView/ItemsView. Mar 27, 2024
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 28, 2024
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Apr 3, 2024
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.

4 participants