launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Review: Approve

Looks good.

[0]

I agree with Francesco's suggestions.  Well, all suggestions but one: I think you should keep "if len(words) > 0:" because it's more specific than using "if words".  For the same reason, if you want to test if "something" is None, we would rather write explicitly "if something is not None:" rather than "if something:".

[1]

147	+        self.assertEqual(
148	+            {'one': True,
149	+             'two': False,
150	+            },
151	+            SampleCommandCollection.parsingParameters())

This is really a detail but I think you could write this as

    self.assertEqual(
        {'one': True, 'two': False},
        SampleCommandCollection.parsingParameters())

It would (artificially) reduce the SLOC and I think it's more readable.  But again, it's really a detail so feel to change it or not.

-- 
https://code.launchpad.net/~ivo-kracht/launchpad/bug-425934/+merge/110786
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.