Code review comment for lp:~pfalcon/linaro-license-protection/crowd-auth

Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

> I guess the "is" on the second changed line should be removed.

Fixed.

> "Name of groups" should be fine, but better have a real-English-speaking person review for this one :-)

Yep, I paused at that too when wrote that, but somehow did it like that (to emphasize that members are of group, not of names?). Let it be just "List of groups".

> Can you please split this on two lines?

Done.

> I know there is nothing wrong with the for loop, but just a suggestion to avoid it:
> return user_groups.isdisjoint(set(required_groups))

Gotta remember (those who will later look at the code too) that Python now has pretty complete set implementation! Another problem is readability and understandability. In particular, in this case, it should be "return not". All in all, it now requires a comment where previously it didn't. Well, ok, let's go for it.

> Hmmm... shouldn't this be bumped to 0.5?

Makes sense.

« Back to merge proposal