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 ListParameter Hashability #2227

Merged
merged 6 commits into from
Sep 12, 2017

Conversation

dlstadther
Copy link
Collaborator

Description

With the merge of #2115, ListParameter normalization uses the _recursively_freeze() method to transform lists of dictionaries into tuples of _FrozenOrderedDict. However, ListParameter's parse() and serialize() methods were not updated to utilize the _DictParamEncoder, allowing JSON serialization.

This PR resolves the serialization of lists of dictionaries provided to ListParameter.

Motivation and Context

Fixes unhashability of ListParameter when provided a list of dictionaries caused by #2115.

Have you tested this? If so, how?

Added hashability test for list of dictionaries.

@dlstadther
Copy link
Collaborator Author

@Tarrasch @j-ostrich Could y'all review? Thanks!

@jonathan-ostrander
Copy link
Contributor

I think the same changes should be made for TupleParameter since its members can also be objects. Other than that, LGTM.

@dlstadther
Copy link
Collaborator Author

TupleParameter and ListParameter only differ in their parse() methods. However, i'm not sure if it makes sense to subclass one of them as the other. Tuples and Lists are similar of use, but not in design or purpose.

Should I subclass TupleParameter as a class of ListParameter, only overriding the parse() method?

@jonathan-ostrander
Copy link
Contributor

I don't see the benefit of subclassing besides saving a couple of lines of code, but I'm also not against it. LGTM, but I don't have write access so you'll have to wait for @Tarrasch to take a look.

@dlstadther
Copy link
Collaborator Author

@joemeszaros I see you added DictParameter. Do you think you could review this? It would be much appreciated! Then i'll merge.

@dlstadther
Copy link
Collaborator Author

I'll merge this tomorrow (2017-09-12) unless I hear otherwise.

@dlstadther dlstadther merged commit ce881b2 into spotify:master Sep 12, 2017
@dlstadther dlstadther deleted the list-dict-hashability branch September 12, 2017 17:58
@joemeszaros
Copy link
Contributor

@dlstadther Sorry for the late response, but I was on holiday and got back just now.

@dlstadther
Copy link
Collaborator Author

@joemeszaros No worries. Feel free to still review (although merged). If you have problems or suggestions, i'm happy to submit another PR with those changes.

This was referenced Jun 29, 2022
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.

3 participants