Thomas Goettlicher write:
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
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
What's your opinion?
Other notes/complains I see on yast report:
- Agree, keep it simple and automatic, so focus only on reading code and discussion about
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.
Software Engineer Appliance Department
SUSE LINUX, s. r. o.
190 00 Praha 9
To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org
To contact the owner, e-mail: yast-devel+owner(a)opensuse.org