Skip to content

added function to tupleize lists inside (nested) dicts #2939

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
merged 4 commits into from
May 11, 2023

Conversation

JustCallMeDavid
Copy link
Contributor

@JustCallMeDavid JustCallMeDavid commented Apr 29, 2023

What was wrong?

Closes #2908

How was it fixed?

Tupleized (nested) lists to render them hash-able inside AttributeDict instances

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

This is Rosie. Adoption profile here: https://www.petfinder.com/dog/rosie-61590987/ny/canastota/wanderers-rest-humane-association-ny79/

@pacrob pacrob force-pushed the hashable-attributedict-lists branch from e1b8942 to 92adaea Compare May 5, 2023 17:29
@pacrob pacrob force-pushed the hashable-attributedict-lists branch from 92adaea to c87efdd Compare May 5, 2023 17:33
@pacrob
Copy link
Contributor

pacrob commented May 5, 2023

Thanks for this @JustCallMeDavid ! I modified the testing a bit and will get the typing/linting sorted out, but the tupelizing function is great!

@JustCallMeDavid
Copy link
Contributor Author

Hi @pacrob, thanks a lot for the review and fixing up the tests, they look a lot better now.

Linting and typing should pass now, but please have a look - not sure if it could not be done better (was having trouble getting the dict assignments to work with more precise types).

@pacrob
Copy link
Contributor

pacrob commented May 9, 2023

Yep, I worked on the typing for a while and couldn't get anything more precise either. I'll get some other eyes on it just to check, but otherwise I think we're good.

@pacrob pacrob requested review from fselmo and kclowes May 9, 2023 19:58
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

I added a comment as some food for thought. I'm good with the current implementation but I think we can make this a bit more of a catch-all for non-hashable types... or at least non-tuple Iterable types to make it a bit simpler. Curious on thoughts there.

@pacrob pacrob force-pushed the hashable-attributedict-lists branch 2 times, most recently from 15ab889 to a7f1b52 Compare May 10, 2023 19:45
@pacrob pacrob force-pushed the hashable-attributedict-lists branch from a7f1b52 to ca74990 Compare May 11, 2023 18:30
Copy link
Collaborator

@fselmo fselmo 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. Thanks @JustCallMeDavid + @pacrob 👍🏼

@pacrob pacrob merged commit 1259fcf into ethereum:main May 11, 2023
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.

web3.datastructures.AttributeDict.__hash__ fails if event logs contain list objects
3 participants