Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally raise detailed exceptions vs. returning False #160

Conversation

haikuginger
Copy link
Contributor

Currently, python-saml has a number of cases where methods simply return False on failure. This prevents error details from bubbling up and being saved for retrieval.

This change decorates those methods with a decorator that, by default, returns False when an exception is raised. However, internally to python-saml, those exceptions are raised and saved for retrieval with get_last_error_reason.

As a result, more detail is available for diagnostics, but the overall API is preserved, because, by default, those exceptions are suppressed when the methods in question are called directly.

Unit tests have been added to ensure that the appropriate exception is raised in all places where False is currently returned when the raise_exceptions=True keyword argument is passed, and existing tests checking that these methods return False when that keyword isn't passed remain as written.

@haikuginger
Copy link
Contributor Author

@pitbulk, we've been using this code in production for a bit without any issues. Any chance we could get it reviewed to see if it fits in here? It doesn't alter the existing API at all; it just opens up some additional options.

@pitbulk
Copy link
Contributor

pitbulk commented Nov 8, 2016

@lorenzogil what do you think about this?

@lorenzogil
Copy link

I personally think exceptions for errors is an improvement over returning booleans and having to check the result of each call I make to a library. With exceptions I can centralize my error checking into fewer parts of my application.

Having said that, this patch is an opt-in feature that the library gives you the choice to take. So I think it doesn't hurt. With testing support it will be not too hard to maintain in the future.

@pitbulk
Copy link
Contributor

pitbulk commented Nov 9, 2016

thanks @lorenzogil

pitbulk added a commit that referenced this pull request Dec 20, 2016
@pitbulk pitbulk mentioned this pull request Dec 20, 2016
@pitbulk
Copy link
Contributor

pitbulk commented Dec 20, 2016

Close it, will merge all in #175

@pitbulk pitbulk closed this Dec 20, 2016
@pitbulk
Copy link
Contributor

pitbulk commented Dec 29, 2016

@haikuginger can you review #175?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants