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.

@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

@spennymac spennymac commented May 6, 2020

#94

Loading

Copy link
Contributor

@plamut plamut left a comment

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.

Loading

@spennymac
Copy link
Contributor Author

@spennymac spennymac commented May 6, 2020

added two tests

Loading

plamut
plamut approved these changes May 12, 2020
Copy link
Contributor

@plamut plamut left a comment

LGTM.

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

Loading

@shollyman
Copy link
Contributor

@shollyman shollyman commented May 15, 2020

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.

Loading

@plamut plamut merged commit 23a173b into googleapis:master May 15, 2020
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants