Mailinglist Archive: yast-devel (52 mails)

< Previous Next >
Re: [yast-devel] Code Review
  • From: Jiri Srain <jsrain@xxxxxxx>
  • Date: Thu, 12 Feb 2015 15:09:13 +0100
  • Message-id: <54DCB409.90703@suse.cz>
On 02/12/2015 03:05 PM, Lukas Ocilka wrote:
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.

Well, if someone really wants to help, he can create PR to the initial branch where the PR is made from .... maybe a bit overkill :-)

Jiri

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



--
Regards,

Jiri Srain
Project Manager
---------------------------------------------------------------------
SUSE LINUX, s.r.o. e-mail: jsrain@xxxxxxxx
Lihovarska 1060/12 tel: +420 284 084 659
190 00 Praha 9 fax: +420 284 084 001
Czech Republic http://www.suse.com
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >