-
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 required score grouping to fellows reviews #113
Conversation
Values have been hardcoded to the given ranks as a more "leaner" way to understand which rank is which. We have pre-emptively added ranks up to dan IX.
That fixed the problems with the tests
Example of error message: Score ruleMissing minimum required score from FellowsRule explanationRule 'Fellows' gives every fellow a score based on their rank, and required that the sum of all the scores is greater than the required score.For more info found out how the rules work in Review-bot types Missing a score of 5Missing 1 in the required score. GitHub users whose approval countsThis is a list of all the Fellows that have not reviewed with their current scores:Users approvals that counted towards this rule with their scores
|
With new fellows the timeout requires to be longer. Extended by 10 seconds
case 8: | ||
return scores.dan8; | ||
case 9: | ||
return scores.dan9; |
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.
could be just return scores['dan' + rank]
?
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'm afraid not unless I convert the object into a collection of keys which I think would make it unreadable.
I prefer to add more lines if they are easier to read than to complicate the code for the sake of reducing its size.
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.
dcdf564
to
b2b80cf
Compare
Jest Test ResultsDuration: 44.257 seconds
Outcome: Passed | Total Tests: 159 | Passed: 159 | Failed: 0
|
b2b80cf
to
4bd69c4
Compare
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.
The PR is very clear to me, well done.
Updated Review Bot to version 2.4.0 which contains paritytech/review-bot#110 (solved in paritytech/review-bot#113) This resolves #93 Updated review bot to latest version which brings new changes to the fellow object: Added new optional field to the config named `scores` that has the score of each fellow per dan (added up to dan IX). When the rule of type `fellows` has the optional field of `minTotalScore` it will check that the score of the fellows who approved the PR sums up to that number. If it doesn't sum to that number it will fail with a custom error listing the score of each fellow for reference (and saying how many points are missing). Find a complete explanation of the new configuration here: [Fellows Rule](/~https://github.com/paritytech/review-bot/blob/v2.4.0/README.md#fellows-rule) - [x] Does not require a CHANGELOG entry I also added some scores to all the dans (feel free to modify them) and updated the `Relay and system files` rule to have a minimum score of 6.
Added new optional field to the config named
FellowScores
that has the score of each fellow per dan. It was done as an object (and not a collection) because it will never change in the foreseeable future (added up to dan IX).When the rule of type
fellows
has the optional field ofminTotalScore
it will check that the score of the fellows who approved the PR sums up to that number. If it doesn't sum to that number it will fail with a custom error listing the score of each fellow for reference (and saying how many points are missing).Added
FellowMissingScoreFailure
class which handles the formatting of errors when the score of fellows is not high enough.This resolves #110
Downgraded JOI as there was a version mismatch that made the test fails.