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

Add __hash__ method for permissions.OperandHolder class #9417

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

vanya909
Copy link
Contributor

@vanya909 vanya909 commented May 30, 2024

OperandHolder is not hashable, so need to add __hash__ method

Description

OperandHolder is not hashable after #8710, need to add __hash__ method to it to provide convenient syntax to filter unique permissions using set or dict.fromkeys

Ways to represent

1. from rest_framework import permissions
2. set([permissions.IsAuthenticated & permissions.IsAdminUser])

image

Fixes

  • Remove __eq__ method from OperandHolder
    or
  • Provide __hash__ method to OperandHolder

Second way is provided in this PR

`OperandHolder` is not hashable, so need to add `__hash__` method
@auvipy auvipy requested a review from a team June 4, 2024 14:02
@auvipy auvipy merged commit fe92f0d into encode:master Jun 10, 2024
7 checks passed
Copy link
Contributor

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

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

lgtm, sorry being slow.

@auvipy
Copy link
Member

auvipy commented Jun 10, 2024

thanks anyway. merged as it was small, after cross checking

@adamchainz
Copy link
Contributor

We should really have tests for this. @vanya909 can you make a follow-up PR with a test that hashes an instance?

@tomchristie
Copy link
Member

Can you explain why using set(...) on these classes is useful?

@vanya909
Copy link
Contributor Author

@tomchristie

Can you explain why using set(...) on these classes is useful?

It's useful in cases when we want to filter out unique permissions in order to avoid calculating of some complex permissions two or more times

Of course it can be done by just iterating over permissions list and appending in a result list those permissions which haven't been added yet, but using set() is more simple

In case when we want to keep original order of permissions dict.fromkeys() can be used, I would say dict.fromkeys() is preferable, but it also requires all permissions to be hashable

@peterthomassen
Copy link
Contributor

peterthomassen commented Jun 16, 2024

Also, this fixes the regression introduced by #8710, so don't think it requires justification beyond that

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.

5 participants