Mailinglist Archive: yast-devel (78 mails)

< Previous Next >
Re: [yast-devel] YaST Code Reviews
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@xxxxxxxx
SUSE
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
References