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

[stdlib] Fix draft for 2756 (not to merge), changes DictEntry.value:V to DictEntry.value: Optional[V] #2851

Closed
wants to merge 3 commits into from

Conversation

rd4com
Copy link
Contributor

@rd4com rd4com commented May 27, 2024

Hello,

This PR is just to help designing a solution for #2756 ,

It also makes the Dict.pop return the DictEntry.value as an owned value by moving the value.

But making it an `Optional could affect the performance, and add complexity to the Dict.

(See feature request #2822 for another approach)

 

The good part is that is seem to fix the #2756, so at least we know what could be the problem.

This PR is quite a design change, but it can be used by staff to see if it fixes some bugs

I don't recommend this solution that adds an Optional, but could be wrong !

Hope it is useful 👍

… to DictEntry.value: Optional[V]

Signed-off-by: rd4com <[email protected]>
@rd4com rd4com requested a review from a team as a code owner May 27, 2024 10:01
… to DictEntry.value: Optional[V]

Signed-off-by: rd4com <[email protected]>
@rd4com
Copy link
Contributor Author

rd4com commented May 27, 2024

⚠️ Tests failed
This is challenging because it fixes #2756 locally on ubuntu

 

See this error:

Command Output (stdout):
--
get: wrong variant type

… to DictEntry.value: Optional[V]

Signed-off-by: rd4com <[email protected]>
@rd4com
Copy link
Contributor Author

rd4com commented May 27, 2024

✅ Tests passed

The change between the two commits is very small. Maybe the order of doing was not correct ?
Can anybody test it on mac with various workers? (1, 2, 3 , 4)

@rd4com
Copy link
Contributor Author

rd4com commented Jun 16, 2024

#2796 should fix that in a better way than this PR, waiting

@rd4com
Copy link
Contributor Author

rd4com commented Aug 10, 2024

#2796 got merged 👍

@rd4com rd4com closed this Aug 10, 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.

2 participants