Code review comment for lp:~mhall119/django-openid-auth/provides-784239

Revision history for this message
Anthony Lenton (elachuni) wrote :

Hi Michael,

I like the approach in general. A few comments/suggestions,
 - you import all defined exceptions on lines 145/151 of the diff, but then you don't seem to use them.
 - does StrictUsernameViolation make sense now? you only use it to inherit from it from two other classes that you raise separately and catch together with all other DjangoOpenIDException. If you think eventually people will want to catch all StrictUsernameViolations together then it's fine as is, otherwise removing it will flatten exception class tree and make it clearer I think.
 - and ideally it would have a test or two I think (to ensure that the messages do indeed reach the user, for example?)
Thanks!

« Back to merge proposal