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 get_all_roles_by_domain api #316

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

BustDot
Copy link
Contributor

@BustDot BustDot commented Aug 31, 2023

refer to: casbin/casbin#1190

@casbin-bot
Copy link
Member

@Nekotoxin please review

@casbin-bot casbin-bot requested a review from Nekotoxin August 31, 2023 01:58
@hsluoyz
Copy link
Member

hsluoyz commented Aug 31, 2023

@leeqvip

for policy in policies:
if policy[len(policy) - 1] == domain:
role = policy[len(policy) - 2]
if role not in roles:
Copy link
Member

Choose a reason for hiding this comment

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

Use set+sort or dict key to list to improve performance.

@BustDot BustDot requested a review from leeqvip August 31, 2023 08:18
if role not in roles:
roles.add(role)

return list(roles)
Copy link
Member

Choose a reason for hiding this comment

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

@BustDot The set is unordered. If the order is not required to get all roles here, then I think it is ok to do so.

@leeqvip leeqvip merged commit 22507ca into casbin:master Aug 31, 2023
@BustDot BustDot deleted the feat/get-all-roles-by-domain branch August 31, 2023 09:42
github-actions bot pushed a commit that referenced this pull request Aug 31, 2023
# [1.26.0](v1.25.0...v1.26.0) (2023-08-31)

### Features

* add get_all_roles_by_domain api ([#316](#316)) ([22507ca](22507ca))
@github-actions
Copy link

🎉 This PR is included in version 1.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants