Pull request procedure

Dear requesting IoTlers,

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 do you think?

Cheers, Oleg

Hey Oleg,

I like your proposed guideline.

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.

Cheers, Cenk

Hi Oleg,

Out of curiosity (and maybe to state the obvious): The rules you proposed would forbid WIP pull requests, right?

Hi,

+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!

Cheers, Martine

Hi Cenk,

Hey,

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

Kaspar

Hi Cenk!

I like your proposed guideline.

Thanks!

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

Cheers, Oleg

[1] Create new page · RIOT-OS/RIOT Wiki · GitHub

Hi Cenk again!

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.

Cheers, Oleg

Hi!

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.

Hey,

@Oleg: Maybe we should continue discussing offline. I choose a baseball bat.

I choose a wasp's nest!

Hey,

Hey,

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.

Hey,

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?

Kaspar

Hey!

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.

Cheers, Oleg

Hey,

Moin,

This might speed up PR reviewing, but it slows PR creation. Let's not

So what. (Less PRs into the pipe) - (more PRs out) = (less open PRs)

Just kidding... :slight_smile:

lose focus, we want to streamline RIOT development, not PR reviewing.

This implies the ancient -and I thought, somehow overcome- missjudgement that testing is not part of the develeopment or at least, kills productivity.

Not kidding...

Paul

Hi again!

So, are there any objections about putting the following into the Wiki?

+1