Code review comment for lp:~jml/launchpad/more-login-helpers

Revision history for this message
Robert Collins (lifeless) wrote :

+ ANONYMOUS are equivalent, and will log the person is as the anonymous

typo - ITYM 'in as'

get_arbitrary should return different members each time I think. At the moment it will give a false sense of arbitrariness.

If you don't like random(), perhaps walking the team list from end to start would do (so that the owner isn't always the first one returned).

Rather than an XXX, I think you've just written some clear explanation of a bit of cruft you're tolerating - thats fine. As I understand XXX's in the lp code base they should be attached to something to fix - and in this case I'd put it on the 'launch bag' saying 'this duplicates the security context and should be deleted'.

Finally, I'd really like a single class rather than these separate functions, but that would be future work, its not needed for this patch - this patch is a clear improvement on its own.

-Rob

review: Approve

« Back to merge proposal