-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added rule type 'fellows' #85
Conversation
and removed the field of rank for non fellows
Removed the tests that evaluate the ranks from the other implementations as now they are only in the type fellows
It was becoming very extended, it was time to update it
@@ -33,11 +34,17 @@ export interface AndDistinctRule extends Rule { | |||
reviewers: Reviewers[]; | |||
} | |||
|
|||
export interface FellowsRule extends Rule { | |||
type: RuleTypes.Fellows; | |||
minRank: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to make this consistent - either camelCase or snake for config or there's some purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer camelCase. Would it be fine if I make a second PR moments after this renaming min_approvals
to minApprovals
?
Created a new type of rule named
fellows
.This rule has its own evaluation (currently, it is mostly a copy of the
basic
evaluation). This is done so we can add special cases for rule, as suggested by @bkchr in polkadot-fellows/runtimes#31 (review).Removed all the ability to request a
rank
in normal rules. I believe that this is better as we now separate the concerns and simplify logic. Theor
,and
andand-distinct
rule didn't really apply on fellows because a higher ranking one belongs also to the lower echelons.This resolves #84 and allows us to experiment better.
Also, this is technically a breaking change, but it has never been applied on any system, so it wouldn't be breaking an existing system.