[yast-devel] Review code, please
Hi, so far only a few people have been commenting on others' commits, and I'd like to ask the others to join the party. We need you! - One obvious reason is sharing the workload (but so far the volume is manageable). - Reading other people's code makes you a better programmer. - Commenting on other people's code improves the code, it prevents ugly code or outright bugs from getting in. - You think it is not your problem? So did I think before inheriting a certain ****ing piece of juicy ****. - Reading *good* code makes me feel better about our work, the project, the product. Or is everyone already reading everything and finding nothing to comment on? -- Martin Vidner, YaST developer http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu
Martin Vidner write:
Hi,
so far only a few people have been commenting on others' commits, and I'd like to ask the others to join the party. We need you!
- One obvious reason is sharing the workload (but so far the volume is manageable). - Reading other people's code makes you a better programmer. - Commenting on other people's code improves the code, it prevents ugly code or outright bugs from getting in. - You think it is not your problem? So did I think before inheriting a certain ****ing piece of juicy ****. - Reading *good* code makes me feel better about our work, the project, the product.
Or is everyone already reading everything and finding nothing to comment on?
Lets see my project proposal for workshop for finding way how to make review patch set explicit, so not review patch set is not in code. Studio guys do it same way, in SLMS there is no explicit rule, but I review everything. Webyast I think not have anything after my move to SLMS. So we should try to find possible ways on workshop and agree on one way ( and we really need it. My proposal is that each patch set should review maintainer (if he is not author), backup maintainer ( to know what happen in module which he can potentially maintain) and master (master in sense of git trunk) keeper, which review general coding rules, documentation, tests and general code quality). You can discuss it with studio guys who already do it and it is quite easy to make a habit from it. Pepa -- Josef Reidinger Appliance Toolkit team maintaining parts of webyast and SLMS author of rubygems - studio_api and net_observer (coauthor) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
Am 28.06.2011 11:42, schrieb Josef Reidinger:
Martin Vidner write:
Hi,
so far only a few people have been commenting on others' commits, and I'd like to ask the others to join the party. We need you!
- One obvious reason is sharing the workload (but so far the volume is manageable). - Reading other people's code makes you a better programmer. - Commenting on other people's code improves the code, it prevents ugly code or outright bugs from getting in. - You think it is not your problem? So did I think before inheriting a certain ****ing piece of juicy ****. - Reading *good* code makes me feel better about our work, the project, the product.
Or is everyone already reading everything and finding nothing to comment on?
Lets see my project proposal for workshop for finding way how to make review patch set explicit, so not review patch set is not in code. Studio guys do it same way, in SLMS there is no explicit rule, but I review everything. Webyast I think not have anything after my move to SLMS. So we should try to find possible ways on workshop and agree on one way ( and we really need it. My proposal is that each patch set should review maintainer (if he is not author), backup maintainer ( to know what happen in module which he can potentially maintain) and master (master in sense of git trunk) keeper, which review general coding rules, documentation, tests and general code quality). You can discuss it with studio guys who already do it and it is quite easy to make a habit from it. Pepa
May be that I am "not uptodate" but I would prefer to read the checkins via a mailing list again. Since we have switched to gitorious this is not possible anymore. How SLMS do this ? Greetings Stefan -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
Stefan Schubert write:
Am 28.06.2011 11:42, schrieb Josef Reidinger:
Martin Vidner write:
Hi,
so far only a few people have been commenting on others' commits, and I'd like to ask the others to join the party. We need you!
- One obvious reason is sharing the workload (but so far the volume is manageable). - Reading other people's code makes you a better programmer. - Commenting on other people's code improves the code, it prevents ugly code or outright bugs from getting in. - You think it is not your problem? So did I think before inheriting a certain ****ing piece of juicy ****. - Reading *good* code makes me feel better about our work, the project, the product.
Or is everyone already reading everything and finding nothing to comment on?
Lets see my project proposal for workshop for finding way how to make review patch set explicit, so not review patch set is not in code. Studio guys do it same way, in SLMS there is no explicit rule, but I review everything. Webyast I think not have anything after my move to SLMS. So we should try to find possible ways on workshop and agree on one way ( and we really need it. My proposal is that each patch set should review maintainer (if he is not author), backup maintainer ( to know what happen in module which he can potentially maintain) and master (master in sense of git trunk) keeper, which review general coding rules, documentation, tests and general code quality). You can discuss it with studio guys who already do it and it is quite easy to make a habit from it. Pepa
May be that I am "not uptodate" but I would prefer to read the checkins via a mailing list again. Since we have switched to gitorious this is not possible anymore. How SLMS do this ?
Greetings Stefan
We have mailing list with commits. Maybe it is time to switch to github, which has some nice options for code reviews, see pull request which allows review whole patch set, so you review whole features or fixes instead single commits - http://help.github.com/send-pull-requests/ I hope project on workshop show all existing solutions and we choose one unified way for whole department ( or at least one for external projects (webyast,yast) and one for internal projects (SLMS,studio,SMT)). -- Josef Reidinger Appliance Toolkit team maintaining parts of webyast and SLMS author of rubygems - studio_api and net_observer (coauthor) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
Martin Vidner write:
Hi,
so far only a few people have been commenting on others' commits, and I'd like to ask the others to join the party. We need you!
- One obvious reason is sharing the workload (but so far the volume is manageable). - Reading other people's code makes you a better programmer. - Commenting on other people's code improves the code, it prevents ugly code or outright bugs from getting in. - You think it is not your problem? So did I think before inheriting a certain ****ing piece of juicy ****. - Reading *good* code makes me feel better about our work, the project, the product.
Or is everyone already reading everything and finding nothing to comment on?
BTW one nice blog article about code review http://thejoyofcode.com/Code_Reviews_are_not_optional.aspx just my notes to stages of code review 1) useless for me, code must say itself what it do 2) really useful, you must see on your own what code do -> code is readable 3) yes, feedback is a must have 4) Revisiting code by code reviewer is for me useless. We should not accept code which is not good enough, so developer must fix bugs before code is merged to master. Pepa -- Josef Reidinger Appliance Toolkit team maintaining parts of webyast and SLMS author of rubygems - studio_api and net_observer (coauthor) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Dne 28.6.2011 13:49, Josef Reidinger napsal(a):
Or is everyone already reading everything and finding nothing to comment on?
BTW one nice blog article about code review http://thejoyofcode.com/Code_Reviews_are_not_optional.aspx
just my notes to stages of code review 1) useless for me, code must say itself what it do 2) really useful, you must see on your own what code do -> code is readable 3) yes, feedback is a must have 4) Revisiting code by code reviewer is for me useless. We should not accept code which is not good enough, so developer must fix bugs before code is merged to master.
== Code Reviews: Just Do It == http://www.codinghorror.com/blog/2006/01/code-reviews-just-do-it.html ~ cut ~ Peer review -- an activity in which people other than the author of a software deliverable examine it for defects and improvement opportunities -- is one of the most powerful software quality tools available. Peer review methods include inspections, walkthroughs, peer deskchecks, and other similar activities. After experiencing the benefits of peer reviews for nearly fifteen years, I would never work in a team that did not perform them. ~ cut ~ L. - -- Lukas Ocilka, Appliances Department, SUSE LINUX s.r.o. MD: Jeff Hawn, Jennifer Guild, Alena Hendrichova -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/ iD8DBQFOCeyQVSqMdRCqTiwRAi3LAJwOc1xTXFyOlaOc0Xp2O+wE07o7ZQCfWZFO qoNGsephd1VxnWmqyB+opLs= =MqDU -----END PGP SIGNATURE----- -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
participants (4)
-
Josef Reidinger -
Lukas Ocilka -
Martin Vidner -
Stefan Schubert