Add Pull Request guidelines#34
Conversation
| @@ -0,0 +1,97 @@ | |||
| # Pull Requests | |||
|
|
|||
| Changes to our repository are to be approved through a peer review process | |||
There was a problem hiding this comment.
Important to note here that this applies for certain branches. In the internal workflow, these PRs should be based on branches that are already in the repo.
Also reference the git guidelines re git-flow and GitHub based workflows. See ADR-001
|
|
||
| ## Preparation | ||
|
|
||
| Speak with your team and/or the repository's maintainer before you open a PR, |
There was a problem hiding this comment.
I think this only applies for big PRs. For small/trivial ones, it is fine to just create the PR and then poke someone.
| For example: Your change uses a particularly complicated implementation for which | ||
| a much more obvious and simple implementation would be possible. However you | ||
| know that we receive inputs that exhibit worst-case run-time behaviour in the | ||
| obvious solution, hence the sophisticated solution is required. In this case |
There was a problem hiding this comment.
In this example, the context should also, or primarily, be in source code comments...
|
|
||
| For example a PR that addresses long outdated comments or clarifies | ||
| documentation with a generally accepted explanation can be handled by Reviewer | ||
| and Reviewee having a short call and agreeing on the change. It is important, |
There was a problem hiding this comment.
I think it is worth explicitly saying here that an exchange on Teams is perfectly sufficient. Especially an exchange in a chat with all relevant stakeholders having visibility.
simondsmart
left a comment
There was a problem hiding this comment.
Minor comments on things that would be nice to have. But happy for you to merge when you are happy with it.
| the obvious solution, hence the sophisticated solution is required. In this | ||
| case you should strongly consider providing enough context within the code as | ||
| comments. Repeating this information in commit messages and PR description will | ||
| go a long way to spread the word. In any case: start with the comments they |
There was a problem hiding this comment.
Check English here: "start with the comments they will read most often and easies to find and closest to the code.". This sentence doesn't make sense.
|
|
||
| For example a PR that addresses long outdated comments or clarifies | ||
| documentation with a generally accepted explanation can be handled by Reviewer | ||
| and Reviewee having a short call or chat with required stakeholders and |
There was a problem hiding this comment.
I really think that this comment needs to the word 'Teams' or 'Online' or similar. Otherwise this still reads as needing to have a synchronous rather than asynchronous communication process.
|
|
||
| - [ ] Do I understand the problem and agree with the solution? | ||
| - [ ] Is the change functionally correct? | ||
| - [ ] Are all GitHub checks passing? |
There was a problem hiding this comment.
Is this part of the process for a reviewer, or for the person doing a merge?
Co-authored-by: Andreas Grafberger <andreas.grafberger@ecmwf.int>
corentincarton
left a comment
There was a problem hiding this comment.
Looks great! Thanks for the work guys @andreas-grafberger and @Ozaq!
Description
Add guidelines how to handle Pull Requests as reviewee and reviewer.
Contributor Declaration
By opening this pull request, I affirm the following: