Thomas Goettlicher write:
Hey developers,
Hi, thanks for start of discussion.
Code reviews examine source code changes in order to find mistakes and increase code quality. YaST code can also profit from systematic code reviews. We should agree on a general way how to do code reviews for changes of YaST code in the future.
From my perspective - code reviews should be optional
Code reviews help keep code clean and reduce number of bugs, so I think that it depends on YaST module. Modules which is expensive to fix later or sensitive like modules in inst-sys or critical parts should have mandatory review, for less critical part it can be optional, but don't forget to another goal of reviews, which is keep module backup up to date.
- everyone should be permitted to read and review foreign code
Yes, open source :)
- everyone should be able to ask for a code review for specific changes
Yes, also good idea, but without assigned person to review certain part of code ( or set of assigned people ) there is risk of no response
- if you change somebody else's code you should ask for a review
I think that if you change someone else code, then you should act like external maintainer so use patches and don't directly fix something in code ( with exception when maintainer is really busy and need help, but this should be agreed on both sides ). This also can help to lower barriers for external contributors.
- nobody should be forced to have his changes reviewed if that causes a backlog (e.g. potential reviewer is busy or on vacation)
Well, code review need not to be before commit, so if potential reviewer is on vacation, then he/she should review after his vacation ( it also help him to keep him/her up to date).
What's your opinion?
Other notes/complains I see on yast report: avoid bureaucracy - Agree, keep it simple and automatic, so focus only on reading code and discussion about it. reward if you review code - I think it is your job...what kind of reward you expect??? mandatory makes fixes more dificult - no, it just increase pressure to avoid hot-fixes which return like boomerang. So fix bugs correctly. Also we often break something when we fix something else, so it can help discover such issue. code reviews for trivial changes don't make sense - even trivial changes can contain bugs, missing doc or missing test case...and trivial changes is easy and quick to review ;) it's up to everyone to review his changes - when everyone review every code, then it is time wasting or no one review anything. Review by another pair of eyes is the most useful, every next review is less efficient and time/improve ratio decrease code reviews are easier if the project is simple and has few developers - it is still simple, just delegate who should review what. YaST is quite good separated to modules, so it is not so hard to assign reviewer to module. So my opinion is that code review: 1) should be mandatory, especially for critical parts 2) should be as simple as possible 3) should be defined who review what ( my idea maintainer every patches, backup should review maintainers code ). If reviewer is unavailable, then he should review it later (after commit, but should review it). What is not clear is what to do if reviewer discover something? What can he do? It is related to my experience when I discover in review one potential issue and coder don't react until he finish and then I get his code and must fix it later. Josef
Cheers, Thomas
-- Josef Reidinger Software Engineer Appliance Department SUSE LINUX, s. r. o. Lihovarska 1060/12 190 00 Praha 9 Czech Republic jreidinger@suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org