Code review comment for lp:~fginther/cupstream2distro-config/fix-pbuilder-hooks

Revision history for this message
Francis Ginther (fginther) wrote :

> I guess this is a matter of preference but I always get confused when I see
> just "True" somewhere:
> 34 + True)
>
> Can you maybe change it to builder_job=True to improve the readability of the
> call?

No problem.

> This is not used anywhere (looks like a leftover from copy&paste):
> 135 + config = {
> 136 + 'hooks': 'parent-hook',
> 137 + 'configurations': {
> 138 + 'raring-amd64': {'node_label': 'pbuilder',
> 139 + 'hooks': 'child-only-hook'},
> 140 + 'raring-i386': {'node_label': 'pbuilder'}}}
>

Removed.

> Not sure if this is the error jenkins complains about but the following fails
> on my machine:
>
> 159 + self.assertEqual(count, 3)
>
> I guess the following should be just +1? :
> 157 + count += 3
>

Oops, I had modified this to make sure it would catch a failure, then forgot to change it back. Fixed.

> Other than it looks good.

« Back to merge proposal