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

Ensure that instances of 'AllPermissionsList' are iterable. #3074

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

tseaver
Copy link
Member

@tseaver tseaver commented Jun 5, 2017

Closes #3073.

@tseaver tseaver requested a review from mmerickel June 5, 2017 21:15
@mmerickel
Copy link
Member

The permissions in the ACLAuthorizationPolicy are allowed to be iterables of permissions. This object is clearly just trying to support that contract such that the object looks like an iterable even if it doesn't define any permissions itself.

The __contains__ is by far the most important method in the policy and is used in

if (ace_action == Allow) and (permission in ace_permissions):
.

Arguably the object itself could just override __eq__ and the policy would wrap it in a list and iterate with ==. I'd argue that the __contains__ + __iter__ is a micro-optimization here. Would you agree @tseaver or do you think the object being iterable is more important? The exposed API is actually ALL_PERMISSIONS which doesn't need to be a "list" at all I think. The class could just as easily be called AllPermissions.

I think this PR is the correct fix if we agree that dropping __contains__ and __iter__ entirely is too much of an incompatibility.

@mmerickel
Copy link
Member

It seems no one wants to talk about that. Ah well.

@mmerickel mmerickel merged commit 5da7f5b into master Jun 9, 2017
mmerickel added a commit that referenced this pull request Jun 9, 2017
@digitalresistor digitalresistor deleted the 3073-all_permissions_list-iterability branch August 1, 2020 22:08
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.

2 participants