-
-
Notifications
You must be signed in to change notification settings - Fork 76
Freeze dictionary keys to make them hashable #228
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
Conversation
This should fix OpShin/opshin-pioneer-program#43 |
Only codecov failed uploading, possibly due to the CI being incorrectly set up |
It looks like there's double recursion in _freeze and _helper, which squares the search depth. Maybe merge the two to freeze just the outer data type, since we can assume the inner values are already frozen. |
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.
This is awesome! 🚀
You're right, the Seth elements and dictionary keys do not need to be frozen again, they can be assumed to be frozen. A frozenhelper function also sounds interesting 🤔 |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #228 +/- ##
==========================================
- Coverage 85.73% 85.56% -0.17%
==========================================
Files 26 26
Lines 2923 2952 +29
Branches 701 707 +6
==========================================
+ Hits 2506 2526 +20
- Misses 307 314 +7
- Partials 110 112 +2
|
I'm looking at this again from my computer (as opposed to my phone), I think I meant to say merge |
@nielstron I made a quick edit here: OpShin#3 |
This might be a good opportunity to use hypothesis to generate arbitrary CBORSerializable types |
If we don't want to freeze all the time we could set a freeze argument and only freeze when true, then set it true recursively for keys but default false otherwise |
I implemented this here: OpShin#4 |
Fix/freeze dict keys
OpShin#5 |
freeze result of to_primitive
This recent PR in frozendict fixes the mypy issue when I tested it locally. Hope they make a new release soon |
OpShin#6 fixes issues with |
use latest frozendict
@cffls This should be ready to merge :) |
Awesome, thanks! LGTM. |
Alternative solution for #227