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

Created BaseEffector class and different methods inherits from it #328

Closed
wants to merge 2 commits into from

Conversation

Akshat111111
Copy link

Added BaseEffector class inorder to remove the duplicate code.Apart from it all the specific classes AllowOverrideEffector, DenyOverrideEffector, AllowAndDenyEffector, and PriorityEffector) inherits from BaseEffector.

Added BaseEffector class inorder to remove the duplicate code.Apart from it all the specific classes AllowOverrideEffector, DenyOverrideEffector, AllowAndDenyEffector, and PriorityEffector) inherits from BaseEffector.
@casbin-bot
Copy link
Member

@Nekotoxin please review

@casbin-bot casbin-bot requested a review from Nekotoxin October 18, 2023 17:11
@CLAassistant
Copy link

CLAassistant commented Oct 18, 2023

CLA assistant check
All committers have signed the CLA.

@hsluoyz
Copy link
Member

hsluoyz commented Oct 18, 2023

@Akshat111111 plz fix CI:

image

Copy link
Member

@leeqvip leeqvip left a comment

Choose a reason for hiding this comment

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

Although it inherits BaseEffector, it is not reused anywhere. So what is this change intended to solve?

@hsluoyz
Copy link
Member

hsluoyz commented Oct 19, 2023

Good point, closed for now

@hsluoyz hsluoyz closed this Oct 19, 2023
@Akshat111111
Copy link
Author

But its reducing the complexity and enhancing the performance as well as readability.

@Akshat111111
Copy link
Author

@Akshat111111 plz fix CI:

image

Yeah I am working on it.

@hsluoyz
Copy link
Member

hsluoyz commented Oct 19, 2023

@Akshat111111 work on exsiting issues, they are more meaningful. Don't create trivial problem by yourself and resolve it just to be a contributor

@Akshat111111
Copy link
Author

Sure I will work on it,But this change I thought will make the code better that's why made this changes.

@hsluoyz
Copy link
Member

hsluoyz commented Oct 20, 2023

@Akshat111111 We normally don't accept trivial PRs and don't want to encourage people to do so

@Akshat111111
Copy link
Author

I understand it, and will now work on the present issues only.

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.

5 participants