-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add documentation for reviewers and a checklist for PRs #315
Conversation
Codecov Report
@@ Coverage Diff @@
## master #315 +/- ##
==========================================
+ Coverage 94.81% 94.94% +0.13%
==========================================
Files 9 9
Lines 848 871 +23
==========================================
+ Hits 804 827 +23
Misses 44 44
|
I know we didn't solve the issues with |
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've some tiny suggestions.
Co-authored-by: Eneko Uruñuela <[email protected]>
Co-authored-by: Eneko Uruñuela <[email protected]>
Co-authored-by: Eneko Uruñuela <[email protected]>
Co-authored-by: Eneko Uruñuela <[email protected]>
Good initiative - this will be helpful. Do you think including something about the timeline of a review would be needed? (Sorry if I've missed this somewhere) I.e. "I know this PR needs to be merged before we can work on X", or "it would be great to get this done by X day". Then a reviewer that is assigned to a few PRs might know how best to spend their time, or they say they don't think they can get it done in that time frame (yes, I might be thinking personally here!). I guess that's kind of what the milestones are for, right? |
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 suggested some changes to your branch @smoia. Other than that, LGTM
Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
Ok, addressed most of the suggestions and questions! |
It seems we are waiting to merge this, after discussion in the next meeting on some of the topics raised, is that correct? |
@tsalo @RayStick @eurunuela I updated a couple of sentences to reflect the discussion we had here and during the last meeting. Please have a look! |
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.
My comments are not critical but I think some of the rephrasing @tsalo suggested should be included.
@eurunuela do you think it's clearer now? |
Sorry I didn't make it clearer before. I commented on the ones I think should be included. |
@eurunuela now? |
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.
LGTM! Thank you!
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 happy with the changes. Thank you.
@RayStick do you mind if we merge this? |
She already approved, so I guess she's happy 🤷🏻♂️ |
We should celebrate this! |
Great. 🎉 |
🚀 PR was released in |
Closes none, depends on #313.
Proposed Changes