Varun,
1. In an earlier revision of user_delete(), you had the code
user_id = kwargs["user_id"]
user = MailmanUser.objects.get(address=user_id)
outside the "if". I don't understand why you moved it inside, and moving the
assignment to user_id inside the "try" seems inappropriate since you have to do
it again after the "if" if the request isn't a POST. This is a minor DRY
violation.
2. The redirect to "user_index" is also repeated (once in the try, once in the
except). I think this can go outside the try, no?
3. What is going to throw an HTTPError inside the try? Surely the assignment
to user_id won't throw an HTTPError. So you should restrict the scope of the
try to those statements that can actually raise the error you're catching.
That's only user.delete(), right? I think the same argument applies to
catching MailmanAPIError. However, Florian seems to be OK with it, so it's
really a matter of style. A possible alternative would be to attack the DRY
violation at the root, by writing a decorator that catches those two errors.
This would be useful because the same pattern appears in other commands like
user_new().
--
https://code.launchpad.net/~sharmavarun/postorius/postorius_gsoc2013/+merge/164011
Your team Mailman Coders is subscribed to branch lp:postorius.
_______________________________________________
Mailman-coders mailing list
[email protected]
http://mail.python.org/mailman/listinfo/mailman-coders
