Skip to content

Add __hash__ method for permissions.OperandHolder class#9417

Merged
auvipy merged 1 commit intoencode:masterfrom
vanya909:patch-1
Jun 10, 2024
Merged

Add __hash__ method for permissions.OperandHolder class#9417
auvipy merged 1 commit intoencode:masterfrom
vanya909:patch-1

Conversation

@vanya909
Copy link
Contributor

@vanya909 vanya909 commented May 30, 2024 •
edited
Loading

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

@vanya909
Add __hash__ method for permissions.OperandHolder class
630c0f7 
`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
peterthomassen
peterthomassen reviewed Jun 10, 2024
View reviewed changes
Copy link
Collaborator

@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
Collaborator

auvipy commented Jun 10, 2024

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

@adamchainz
Copy link
Contributor

adamchainz commented Jun 12, 2024

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

@vanya909 vanya909 mentioned this pull request Jun 14, 2024
Merged
@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Jun 14, 2024

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

@vanya909
Copy link
Contributor Author

vanya909 commented Jun 14, 2024

@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
Collaborator

peterthomassen commented Jun 16, 2024 •
edited
Loading

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

Reviewers

@peterthomassen peterthomassen peterthomassen left review comments

@auvipy auvipy auvipy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vanya909 @auvipy @adamchainz @lovelydinosaur @peterthomassen