A more hands on review process

[Stephen Kelly]

Tom Albers wrote:

> Hi Stephen,

Hey,

> I just have to disagree. ;-) I think the current system works perfectly,
> is your improvement based on any kind of conversations with people or is
> it a static observation? After applications get moved to kdereview and it
> gets announced, I regulary see someone fixing the build system, checking
> of there is a messages.sh, etc, etc.

My suggestion is based on static observation of k-c-d review threads. I
didn't consider IRC based feedback or feedback on the module mailing list,
which I'm sure happens as part of the development process anyway. I also
didn't consider review and improvement by devs after code gets into
kdereview as you describe. The reason for that is I didn't think to
consider them (short-sightedness on my part), and mostly I can't see
them :).

> 
> I think the current system works ok. If you base it on criteria, one has
> to wait untill all checkboxes are checked, which can take more time (read:
> forever), so in that case you also need to define, who checks what.

I'm not sure it needs to be a checklist which all has to be green before the
reviewer(s) calls it good-to-go, but a list of suggestions for the reviewer
to consider and report on. The reviewer might cut some of my list out, and
add his own criteria, but other readers would simply know what his/her
criteria were, and know for example that someone considered the
security/portability implications of the new code.

The main thing I'd like to see less of is review announcements which go
without a reply, or which get short non-specific replies and you're not
sure how much the reviewer looked at the code and what was considered when
OKing it.

> I rather not go on that path. So, although your proposal is a more
> 'official' review and might be better, I think the current system is good
> enough and does it's job.

Yes, I think the review process is already quite effective, and making it
more 'official' would always be a difficult thing for a volunteer project.
That's the reason all criteria are optional.
 
> Besides, ticking boxes does not tell anything about the quality either. An
> i18n review by Albert is much more valuable than a review in that area by
> me.

Yes, true. However, you or I could say with some confidence that:

* One word strings use context for translation
* Strings are free from word puzzles
(http://techbase.kde.org/Development/Tutorials/Localization/i18n_Mistakes)
* The KUIT system is not
usedhttp://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics.
That's OK for now, but an area for future work.
 
>> If this is seen as a good idea, I'd like to get feedback on the criteria
>> I've suggested below. Anything covered by krazy is probably not necessary
>> to be on this list (eg, licence, copyright information).
> 
> The list below is indeed a nice list for people to look at before they
> start moving applications to kdereview. We should point to a page listing
> that from the svn_guidelines page.
> 

I'll try to put this checklist or a variation of it on that page or a linked
page. I'll make clear that it's only things for the developer to keep in
mind, and not an 'All must be green' type of list. 

Thanks to everyone who commented on the 'checklist'.

So, in summary, this is probably more a suggestion of a social change in
calling for reviewers to record the criteria they're reviewing, rather than
something else for developers of new applications and libraries to worry
about. Probably a far more difficult task, but something that can be
encouraged.

Cheers,

Steve.

> Best,
> 
> Toma