[Cubicweb] Cubicweb's patch revisions

Nicolas Chauvat nicolas.chauvat at logilab.fr
Tue May 31 10:53:48 CEST 2011


Hola Carlos,

On Mon, May 30, 2011 at 03:42:59PM -0500, Carlos Balderas wrote:
> 1.- Check if the patch is linked to a ticket. Not in my case. I guess
> this is to get more information and understand better what
> the patch does.

The goal is also to track changes and make sure we know what was
altered in a version. If there is no ticket, you just tell the author
of the patch "the patch is not linked to a ticket", you do not fix the
problem yourself.

> 2.- Check if the commit messages and comments are enough to understand
> the change. In case it isn't clear enough, we should write some
> comments to ask for more information, right?

Yes. You could for example say "the patch has a one line comment,
could you explain a bit more ?".

> 3.- Review the code looking for pitfall/bugs. I see this can be done
> in cubicweb forge, looking at the version content of each patch.
>
> At this point, I have checked the patch at first sight.

Looking at the patch is good enough.

> but what I would really like to do is to test it. Here is where it
> comes some other topics.
 
You can do this, but it is more work. The author of the patch is not
supposed to send something that does not work. The automated test
suite run by apycot should catch problems if the patch is applied.

> So I go looking for the patch, and apply it on cubicweb.
> 
> 1.- How do I know what branch (stable, default, other) should I use to
> apply the patch?

Some authors follow the convention stable_something.diff or
default_something.diff

> 2.- I would think running cubiweb's tests with pytest tool would be
> the least I can do, right?
> Then I go to the test folder and start to run the test with pytest,
> but I am not sure I am doing this right, because the tests don't
> return messages

Maybe a PYTHONPATH problem ?

> 3.- Well, let's say that tests ran ok, but at this point the tests I
> made could've not even touch the code of the patch yet, nor to see
> if there is any problem to run it, nor to test the logic of the
> change. This left me the feeling of the patch hasn't been checked at
> this time.

You are turning code review into something much more difficult (and
complete) than intended. As a reviewer, your role is to make sure the
patch that will go on the pile of the committer is of sufficient
quality to be processed. You are supposed to make it easy for the
committer to say "go" or "no-go". We have very few committers, so we
want to make sure their time is put to good use and they do not spend
their day sending emails to say "your patch is not linked to a
ticket". You do not have to run every possible test to make sure the
patch will not break anything.

Does that help ?

-- 
Nicolas Chauvat

logilab.fr - services en informatique scientifique et gestion de connaissances  



More information about the Cubicweb mailing list