search

mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
179 stars 80 forks source link

Log the exception (it was silent) #180

Closed seocam closed 11 years ago

seocam commented 11 years ago

"... Errors should never pass silently. ..." (The Zen of Python, by Tim Peters)

Osmose commented 11 years ago

Thanks for the pull request!

This looks alright, but it doesn't pass the test suite because it fails if error is None. You should probably add a check for that prior to accessing it (Using getattr or something). Also, it would be nice if there was a test to make sure if this logs the exception if it exists.

If you won't be able to make these fixes, let us know and we'll eventually make the fixes ourselves (or someone might be amazing and make the changes and resubmit). Thanks again for the patch!

seocam commented 11 years ago

Sorry for breaking the tests :)

Fix made and test written.

seocam commented 11 years ago

Just to clarify, would you like me to add an unicode method to BrowserIDExecption and update login failure doctoring? I can do that. Cheers!

Osmose commented 11 years ago

Just adding __unicode__ and logging the exception directly is fine. You're right about the docstring mandating that the exception be a BrowserIDException, so at this point it's just a matter of the code being cleaner. :D

PS: Thanks for following up on this, you're a better person than I. :+1:

seocam commented 11 years ago

No worries! :) I can see from all your activity that you have been very busy. Anyways, the pull-request was updated. Cheers!

Osmose commented 11 years ago

Merged in a2174ec7685c5dd6014b48015fafb9bd73328518. Thanks for the patch!