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

Freeze dictionary keys to make them hashable #228

Merged
merged 23 commits into from
Apr 30, 2023

Conversation

nielstron
Copy link
Contributor

Alternative solution for #227

@nielstron
Copy link
Contributor Author

This should fix OpShin/opshin-pioneer-program#43

@nielstron
Copy link
Contributor Author

nielstron commented Apr 25, 2023

Only codecov failed uploading, possibly due to the CI being incorrectly set up

@juliusfrost
Copy link
Contributor

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.

Copy link
Collaborator

@cffls cffls left a comment

Choose a reason for hiding this comment

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

This is awesome! 🚀

@cffls cffls added the enhancement New feature or request label Apr 25, 2023
@nielstron
Copy link
Contributor Author

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.

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-commenter
Copy link

codecov-commenter commented Apr 25, 2023

Codecov Report

Merging #228 (ddb6b93) into main (da6da64) will decrease coverage by 0.17%.
The diff coverage is 65.78%.

📣 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     
Impacted Files Coverage Δ
pycardano/serialization.py 84.61% <65.78%> (-2.32%) ⬇️

... and 3 files with indirect coverage changes

@juliusfrost
Copy link
Contributor

I'm looking at this again from my computer (as opposed to my phone), I think I meant to say merge _dfs with _frozen

@juliusfrost
Copy link
Contributor

juliusfrost commented Apr 25, 2023

@nielstron I made a quick edit here: OpShin#3

@juliusfrost
Copy link
Contributor

This might be a good opportunity to use hypothesis to generate arbitrary CBORSerializable types

@juliusfrost
Copy link
Contributor

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

@juliusfrost
Copy link
Contributor

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

pycardano/serialization.py Outdated Show resolved Hide resolved
@juliusfrost
Copy link
Contributor

OpShin#5
Fixes TypeError: unhashable type: 'list'

@juliusfrost
Copy link
Contributor

This recent PR in frozendict fixes the mypy issue when I tested it locally. Hope they make a new release soon

@juliusfrost
Copy link
Contributor

OpShin#6 fixes issues with poetry run mypy --install-types --non-interactive pycardano

@juliusfrost
Copy link
Contributor

@cffls This should be ready to merge :)

@cffls
Copy link
Collaborator

cffls commented Apr 30, 2023

Awesome, thanks! LGTM.

@cffls cffls merged commit 93fd889 into Python-Cardano:main Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants