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

feat: add a __hash__ implementation to AccessEntry #93

Merged
merged 5 commits into from
May 15, 2020

Conversation

spennymac
Copy link
Contributor

@spennymac spennymac commented May 6, 2020

added __hash__ to AccessEntry

Closes #94.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 6, 2020
@spennymac spennymac changed the title Add a __hash__ implementation to AccessEntry feat: add a __hash__ implementation to AccessEntry May 6, 2020
@spennymac
Copy link
Contributor Author

#94

@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 6, 2020
@IlyaFaer IlyaFaer requested a review from shollyman May 6, 2020 14:54
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 6, 2020
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

The __hash__() implementation looks good, let's just add a few tests covering it, e.g. making sure they can be added to (and located in) sets and dicts, equal instances having the same hash, etc.

The only potential concern is that the change makes the AccessEntry instances immutable (required with this hash implementation). I wonder if there is any user code that actually mutates these instances, as this change will break it? (cc: @shollyman)

Nevertheless, it's a simple class similar to DatasetReference, which is already hashable (and immutable), and its main purpose is just to group a few values, thus making AccessEntry immutable for hashability seems reasonable IMO.

@spennymac
Copy link
Contributor Author

spennymac commented May 6, 2020

added two tests

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 7, 2020
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 11, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 11, 2020
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

LGTM.

Will wait with merging, though, until the product gives a green light, as this is a potentially breaking change for some users.

@shollyman
Copy link
Contributor

The pattern I can think of where mutable ACE mutations be leveraged is for code that amends an ACL via dataset update. There are cases where walking the existing ACL to amend the entries might be happening via direct mutation rather than replacing via a newly constructed ACE.

I tried to search for code examples that mutate ACEs in place, but I didn't find anything with a coarse pass. The much more common idiom is appending an entry to an existing ACL. I can't think of any existing integrations that would do this, it feels like a very specific action.

This feels relatively low risk, and not sufficient to necessitate a major rev in release to me. Peter, let's make sure we call this out clearly in the release notes however.

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2020
@plamut plamut merged commit 23a173b into googleapis:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make AccessEntry hashable
6 participants