in order to improve and hopefully speed up the Pull request/review process, I
think it would be beneficial to describe in a better defined way how a Pull
request should be created and maintained. Therefore, I plan to put the
following rules into the wiki:
* The title and initial description of a Pull request must describe its basic
idea and what goal is intended to be achieved in a brief and comprehensible
manner.
* The provided code and its documentation should make it very clear how this
goal is intended to be solved.
* Keep Pull requests as small as possible. The smaller a PR, the more likely
it gets reviewed in short time.
* Support your reviewer! Try to react as quick as possible to your reviewer's
comments - and if only by letting her/him know, that you have currently no
time to incorporate her/his feedback. Also, let the reviewer know if you do
not plan to continue to work on a certain PR. Furthermore, if your reviewer
don't react for some days, remind him!
What's your opinion on adding some words about logically splitting a PR across several commits. I always like it when a PR contains 1 commit for the new feature / bugfix and 1 commit for new/modified (unit)tests. This way I can review them separately in github.
+1 for your guideline (though I really need to put some work into point s 1 and 3 ^^).
On the somewhat-positive-feedback-side for the maintainers and reviewers in general: If I saw and remember correctly we, brought the number of open PRs of ~170 in the last few weeks finally down to ~140 again. Now let’s get below 100 again!
* The provided code and its documentation should make it very clear how this
goal is intended to be solved.
This seems like a huge burden to developers writing non-trivial code.
What does "very clear" mean?
Proper reviewing includes reading and understanding the actual code. If
code is unclear, reviewers ask for clarification. Now, apart from a
concept that has been approved, perfect doxygen, commented code, there
needs to be code meta documentation that makes "very clear" how the code
works.
This might speed up PR reviewing, but it slows PR creation. Let's not
lose focus, we want to streamline RIOT development, not PR reviewing.
What's your opinion on adding some words about logically splitting a PR
across several commits. I always like it when a PR contains 1 commit for the
new feature / bugfix and 1 commit for new/modified (unit)tests. This way I
can review them separately in github.
Good idea. Although it should be already covered by point 7 of the Development
Procedures [1].
Out of curiosity (and maybe to state the obvious):
The rules you proposed would forbid WIP pull requests, right?
No, as Martine stated, I don't see a reason why this should prevent WIP PRs.
However, in my personal work flow I handle WIP PRs similar to Martine: mostly
ignore them. I think WIP PRs should stay an exception (as they have been until
now). They are mostly needed if you want to show a new concept to the rest of
the community to ask for feedback, before you actually flesh out the stuff.
This seems like a huge burden to developers writing non-trivial code.
What does "very clear" mean?
Proper reviewing includes reading and understanding the actual code. If
code is unclear, reviewers ask for clarification. Now, apart from a
concept that has been approved, perfect doxygen, commented code, there
needs to be code meta documentation that makes "very clear" how the code
works.
This might speed up PR reviewing, but it slows PR creation. Let's not
lose focus, we want to streamline RIOT development, not PR reviewing.
Now it's up to me to ask: are you trying to piss me off?
I see this actually as one of the most important rules. If the code is complex
and poorly documented, it sucks. Full stop!
Do you know how many hours I spent in trying to understand poorly documented,
but complex (and not always brilliant) code? And do you know how many times I
did this not only once, while reviewing, but hundreds of times while debugging
code or reviewing new PRs that changes this code? Sorry, Kaspar, but I won't
review any poorly documented code any more. It's waste of time.
Cheers,
Oleg
P.S. Sorry for the harsh words, but this attitude drives me nuts.
One argument for decently documented/explained code is that it is not only for the reviewer(s) before merging, but for ALL the people who will look at the code and try to understand/modify/extend it AFTER it’s been merged… That’s potentially a LOT of people… and a lot of problems down the road, potentially.
One argument for decently documented/explained code is
Do I sound like I want to write complex, undocumented code?
I just don't agree with a "rule" requiring a PR to have perfect
documentation before even opening it.
Documented code (might be self-documenting) is a baseline that we all
agree with. Having that as a rule for PRs is IMHO overregulation.
Say I write a complex piece of code, add comments where I find them
necessary. As I wrote the complex beast, I find everything "very clear".
So I open the PR, and someone says "sorry I don't understand and
documentation is missing".
So I improve documentation and try to clarify things until the reviewers
are happy.
That's how we've been doing things as far as I can remember.
Now of what use is a rule requiring "documentation that makes things
very clear"? What does "very clear" mean?
Say I write a complex piece of code, add comments where I find them
necessary. As I wrote the complex beast, I find everything "very clear".
So I open the PR, and someone says "sorry I don't understand and
documentation is missing".
So I improve documentation and try to clarify things until the reviewers
are happy.
Fully agree.
That's how we've been doing things as far as I can remember.
If I look at large pieces of our code, I have to admit: unfortunately not. I
can only speak for me, but I believe that others made the same mistake:
reviewed code, spent (wasted) a lot of time to understand it, and failed to
ask for (better) documentation.
Now of what use is a rule requiring "documentation that makes things
very clear"? What does "very clear" mean?
Am I mistaken or do you react somehow allergic to the word "rule" here? It's
more like a guideline in a sense that one should try the best you can to make
the reviewer's job easier. If you think your code is well documented and
understandable, fine, you're all set!
Actually, this "rule" (or however you wanna call it) is more a reminder to
both parties: for the pull requester to think about documenting the code and
for the reviewer to ask for this documentation if it takes too much time to
understand it. Of course, "well documented" and "very clear" are blurry,
subjective terms, but if everyone tries to implement her/his personal
understanding of these terms, we're already in a much better position.