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

Converting properties list to a string before it is input into hash. #154

Closed

Conversation

0t1s1
Copy link

@0t1s1 0t1s1 commented Jan 28, 2022

Resolved isssue #153 .

Signed-off-by: Aaron Otis [email protected]

@0t1s1 0t1s1 requested a review from a team as a code owner January 28, 2022 02:14
Copy link
Member

@jkowalleck jkowalleck left a comment

Choose a reason for hiding this comment

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

i dont like the the proposed fix for #153 for the following reasons:
converting an object to string does not solve the problem.

is there a reason why the classes that currently fail hashing should not have a __hash__() implemented?
current fix is a yet another shortcoming, that fights symptoms of other shortcoming.

@0t1s1
Copy link
Author

0t1s1 commented Jan 31, 2022

The classes that are currently failing to hash are lists, which do not have __hash__() implemented because they are mutable. I agree that the purposed fix isn't the best, I just copied what was already being done for the other three lists stored in the object. Converting the lists to strings is not a solution as identical Component objects will not be evaluated as equal if the underlying lists are in different orders.

I was considering opening a different PR to address this, but if you like we can tackle it in this one. It appears this has been addressed in the comments of #153.

@madpah
Copy link
Collaborator

madpah commented Feb 2, 2022

Thanks for the PR @0t1s1 - but we already have a proper solution (we hope) in #148 if you care to take a look there and provide any feedback?

@jkowalleck jkowalleck mentioned this pull request Feb 2, 2022
@0t1s1
Copy link
Author

0t1s1 commented Feb 3, 2022

Thanks @madpah. I took a brief look at #148 and it looks fine to me. Do you want me to close this PR?

@madpah
Copy link
Collaborator

madpah commented Feb 8, 2022

If you're happy to close this PR @0t1s1, that would be helpful.

Thanks for calling this out - we're hoping to get 2.0.0 released next week which includes this update.

@0t1s1 0t1s1 closed this Feb 8, 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