Mailinglist Archive: yast-devel (52 mails)

< Previous Next >
Re: [yast-devel] Code Review
On 12.2.2015 12:01, Steffen Winterfeldt wrote:
On Thu, 12 Feb 2015, Stefan Schubert wrote:

Hi,
the machinery team uses pull requests for doing code refactoring.
Have a look to:

http://tech.lovewithfood.com/blog/2014/09/04/refactoring-in-code-review-my-experience/


About 80 percent of my pull requests are commented with suggestions how
to improve
the code and every time I am thinking on my own: "Hey, nice, but why
have you not already
changed it immediately in my pull request?"

[...]

What do you think ?

I'd rather hit him on the head if someone did this. ;-)

But seriously, the point of pull requests is that someone _else_ is looking
at it. If that someone would change himself, again someone would have to
look at it, etc...

I have to say that I agree to Steffen's understanding of a default pull-request concept. Except that "hitting someone" to their head ;)

Code review, as we understand it now, is here to establish a process that should keep bugs out of our code and to check that the code conforms to given standards (tests, variable names, methods length, ...).

But of course, additionally, it's here for learning from each other. On the other hand, everyone's opinion is the same as anyone else's opinion (but yes, some people are more skilled than others), so when doing a code-review, you as a reviewer should not overrule the original author's idea. Reviewer should do their best to suggest fixes and enhancements.

At the end, if the reviewer wants, they can create their or PR with their own enhancements right after the original PR is done if it's worth it.

Fact is, you're the author of that change and you're responsible. So others
may comment but not change.

That's on the other hand something else. Ownership and responsibility should not be limited to the code that a developer has created. I myself, for instance, feel the responsibility for all the code that my team develops and maintains, although I have implemented just some parts here and there.

At the end, maintainers change, and there is also code that is maintained by "someone who just have time for that". If the code is well written - understandable to others, then virtually anyone can step in and fix / enhance it.

Schubi asks for a special code-creation process, something more like a pair-programming or rather a two-man saw programming - people work one by one in iterations to reach a given goal. Such process, on the other hand doesn't seem to fit the original code-review idea though, but it seems to be worth trying anyway.

1. The PR author needs to specify that this is a two-man sawing process
2. The PR needs to be developed in a special branch in the original repository (instead of author's fork) [or you know something better?]
3. It has to be defined in which circumstances a PR can be merged - when it's finished - and this needs to be know at the beginning, and who can actually merge it

This would bring another level of cooperation and could speed up some PRs. On the other hand, it doesn't seem to be "the" default process for PRs.

Thanks for bringing this up
Lukas

--

Lukas Ocilka, Systems Management (Yast) Team Leader
SLE Department, SUSE Linux
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >