[yast-devel] Code Review
From my point of view I would really appreciate if the person who reviews my pull requests would also change the code at once if he/she see any improvements. So
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-e... 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?" the reviewer does not have to ask for permissions. Just do it. ;-) Sure, I would have to test the changed code again. But that is not really a problem:-) What do you think ? Greetings Stefan -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Thu, 12 Feb 2015 09:18:50 +0100
Stefan Schubert
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-e...
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?" From my point of view I would really appreciate if the person who reviews my pull requests would also change the code at once if he/she see any improvements. So the reviewer does not have to ask for permissions. Just do it. ;-) Sure, I would have to test the changed code again. But that is not really a problem:-)
What do you think ?
Greetings Stefan
Hi Stefan, for me there is two main reasons why not to do it: 1) not all changes are non-controversial, so changing something without agreement of code author do not see for me like nice stuff. And sometimes it is even hard to recognize what is controversial change. 2) learning part will be missing. If I write in comment better ruby construct, then I think it is more educative if author look at it, think about it and if it found better also try to write it themself. I worry that if I start writting changes myself, then in the end, I need to rewrite all pull requests I will see :) Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
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-e...
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... Fact is, you're the author of that change and you're responsible. So others may comment but not change. Steffen -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
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-e...
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@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
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-e...
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@suse.com 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@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Thu, Feb 12, 2015 at 03:05:18PM +0100, Lukas Ocilka wrote:
On 12.2.2015 12:01, Steffen Winterfeldt wrote:
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, ...).
I don't see how the code review done here keep bugs out. Maybe they keep some bugs out. But to generally keep bugs out would require the reviewer to know and understand the existing code which is often not the case.
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.
From my understanding the persons who have to fix bugs and implement features have the ownership and responsibility. Often that is just one person.
Regards,
Arvin
--
Arvin Schnell,
On 12.2.2015 15:24, Arvin Schnell wrote:
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, ...).
I don't see how the code review done here keep bugs out. Maybe they keep some bugs out. But to generally keep bugs out would require the reviewer to know and understand the existing code which is often not the case.
OK, so what do you suggest to make it even better? Please, consider the current team setup, knowledge and tasks. I'm always open to good ideas.
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.
From my understanding the persons who have to fix bugs and implement features have the ownership and responsibility. Often that is just one person.
Yes, and I'm constantly pushing for changing this as one-guy-for-one-module doesn't scale. For instance, see how Ancor has been working for several months now. See where Christopher is heading to. Check out what Ladislav and Josef have been doing in the Travis/RuboCop area. E.g., I've asked for a new Libstorage developer position and I've got it. I'm currently in the process of hiring new developer. The same for AutoYast. My request for Linuxrc position hasn't been approved yet. Anyway, these new people *will not* work only on libstorage/autoyast/linuxrc. I want them take the responsibility for the code that we own as a team. -- Lukas Ocilka, Systems Management (Yast) Team Leader SLE Department, SUSE Linux -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Thu, 12 Feb 2015, Lukas Ocilka wrote:
On 12.2.2015 12:01, Steffen Winterfeldt wrote:
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.
Actually, I meant more the responsibilty for the pull request. It would be nice if there's a single person that understands the patch and that is the person git logs as the one doing the commit. Because then I know who to ask about the change years later. Steffen -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 13.2.2015 12:19, Steffen Winterfeldt wrote:
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.
Actually, I meant more the responsibilty for the pull request. It would be nice if there's a single person that understands the patch and that is the person git logs as the one doing the commit. Because then I know who to ask about the change years later.
OK, I see your point. But this works only short-to-mid term. A few years later, people might be somewhere else already and if not, they will probably not remember anyway. The best is, if the whole change was self-descriptive. If all the commits had a decent description and if the pull-request described the circumstances of the whole change, including the bug or FATE number. Additionally, that's why Martin insists on understandable description in .changes file - Now we might understand it, but we'll be in different situation in a few months or years. Anyway, I agree that by default, having a single person "feeling the responsibility" for a given change (PR) makes sense. Thanks Lukas -- Lukas Ocilka, Systems Management (Yast) Team Leader SLE Department, SUSE Linux -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
Am 13.02.2015 um 12:49 schrieb Lukas Ocilka:
Additionally, that's why Martin insists on understandable description in .changes file - Now we might understand it, but we'll be in different situation in a few months or years.
Anyway, I agree that by default, having a single person "feeling the responsibility" for a given change (PR) makes sense.
Thanks Lukas
OK, I see that there are more cons than pros. So, let's close this discussion and keep the current state. I am fine this too:-) Thanks for the input !!!! Greetings Stefan -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
participants (6)
-
Arvin Schnell
-
Jiri Srain
-
Josef Reidinger
-
Lukas Ocilka
-
Stefan Schubert
-
Steffen Winterfeldt