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

Added required score grouping to fellows reviews #113

Merged
merged 19 commits into from
Jan 29, 2024
Merged

Conversation

Bullrich
Copy link
Contributor

@Bullrich Bullrich commented Jan 29, 2024

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 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).

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.

@Bullrich Bullrich added the Fellows Tickets related to the fellowship label Jan 29, 2024
@Bullrich Bullrich self-assigned this Jan 29, 2024
@Bullrich Bullrich requested a review from a team as a code owner January 29, 2024 13:04
@Bullrich Bullrich linked an issue Jan 29, 2024 that may be closed by this pull request
@Bullrich
Copy link
Contributor Author

Example of error message:

Score rule

Missing minimum required score from Fellows

Rule 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 5

Missing 1 in the required score.
Current score is 4/5

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

case 8:
return scores.dan8;
case 9:
return scores.dan9;
Copy link
Contributor

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] ?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

but i see your point

src/runner.ts Show resolved Hide resolved
src/polkadot/fellows.ts Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 29, 2024 13:22
@Bullrich Bullrich requested a review from mordamax January 29, 2024 13:33
Copy link

Jest Test Results

Generic badge

Duration: 44.257 seconds
Start: 2024-01-29 13:39:44.093 UTC
Finish: 2024-01-29 13:40:28.350 UTC
Duration: 44.257 seconds
Outcome: Passed | Total Tests: 159 | Passed: 159 | Failed: 0
Total Test Suites: 16
Total Tests: 159
Failed Test Suites: 0
Failed Tests: 0
Passed Test Suites: 16
Passed Tests: 159

Copy link
Contributor

@rzadp rzadp left a 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.

@Bullrich Bullrich enabled auto-merge (squash) January 29, 2024 16:29
@Bullrich Bullrich merged commit 2800183 into main Jan 29, 2024
11 checks passed
@Bullrich Bullrich deleted the fellows-score branch January 29, 2024 16:30
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request Feb 14, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fellows Tickets related to the fellowship
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add score grouping to fellows reviews
3 participants