clicompanion-devs team mailing list archive

[Jonathan Marsden <jmarsden@xxxxxxxxxxx>]

On 11/18/2011 05:22 AM, bdfhjk wrote:

> 1. Changes related to package were proposed by DD, so I assume that
> they are good and not need additional review.


I've not looked through them all, but the debian/NEWS one surely needs
careful review ... I feel strongly it needs rejecting because it is in
the wrong bzr tree -- it should be in the application tree, IMO.  I
provided some pointers to standards documents (Debian Policy and the
Developer Manual) about this earlier.

> 2. Changes regarding turning off colors are very small and they in my
> opinion not need review.


No comments seem to have been added to the code about the change.  The
removed code was left present but commented out.  Both seem unusual
coding style choices to me... if you leave the old code in, then I'd
suggest at least adding a one line comment something like

# Code to set default colors commented out.  Fixes LP bug #123456

before each chunk of changed lines.

However, since the codebase is in a version control system (bzr), what
purpose does it serve to leave comment out code in there at all?  How
does it help the project to leave those in there?  If someone needs to
see the history of that code, they can use bzr diff, or check out the
previous revision of each file, or use a pretty web based viewer of the
bzr repository to highlight the changes for them :)

Therefore, I think it would be cleaner (and so preferable) to completely
remove all the lines of code that were commented out by these revisions.

Lastly on this one, it might have been more appropriate to provide for
colour customization, perhaps via a clicompanion configuration file,
rather than just removing the hardcoded defaults and always relying on
the underlying DE theme for colour choices -- was this design approach
considered and rejected?

This is the kind of information and recommendations that I would expect
us to provide each other provide in reviews, if we were doing reviews of
these changes :) :)

Do you still think these changes were so perfect that they would not
benefit from being reviewed?  Please do not misunderstand: I am glad you
did the work.  It was not "bad", or "wrong", or anything like that.  It
is just that we can be more effective if we work together, and that
includes reviewing each other's work.

> If anyone else want (please write to me) - then I will report and
> request review of any small changes (even confirmed by DD) - but that
> slow our work  (remember, that Duane is a very busy man)


Any of us on the development team can do reviews of any work done by any
other person on the team.  All of us have merge rights, so why would
Duane being busy be a bottleneck here?

The current major bottleneck to merging into trunk and debian is not
Duane, it is *you* :)  You froze both of them around the 5th of
November, so right now, no-one is supposed to merge anything into either
of them, until you tell us the 1.1 release freeze is over.  A two week
freeze is pretty long already... when do you expect to release 1.1 and
the Debian package of it?

Jonathan