Skip to content

raising PreventUpdate should not print to stderr, and PreventUpdate should not inherit Exception #516

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

Closed
0zeroth opened this issue Dec 27, 2018 · 4 comments
Assignees

Comments

@0zeroth
Copy link

0zeroth commented Dec 27, 2018

Hi, I think the default implementation should not print to stderr when PreventUpdate is raised. This is a minor point, because the following is a work-around (and as the docs improve both PreventUpdate and example of a custom error handler could be documented!)

# Example of replacing the default error handler for PreventUpdate
app = dash.Dash(__name__)

@app.server.errorhandler(dash.exceptions.PreventUpdate)
def _handle_error(error):
    """Handle a halted callback and return an empty 204 response"""
    #print(error, file=sys.stderr)
    return ('', 204)

Also, as a minor point, PreventUpdate should probably inherit BaseException without inheriting Exception - the reason is the same as why StopIteration does not inherit Exception (because its not an error).

@T4rk1n
Copy link
Contributor

T4rk1n commented Dec 27, 2018

From https://docs.python.org/3/library/exceptions.html#BaseException

The base class for all built-in exceptions. It is not meant to be directly inherited by user-defined classes (for that, use Exception).

The print statement should be removed.

@alexcjohnson
Copy link
Collaborator

@T4rk1n I'm not convinced one way or the other, but one practical benefit of making PreventUpdate inherit only from BaseException is that users can make try: ... except Exception as e: ... blocks and have PreventUpdate still pass through - because, as @0zeroth said, it's not really an error so you probably aren't going to try to catch it. Can you think of any problems this would cause?

@T4rk1n
Copy link
Contributor

T4rk1n commented Jan 20, 2019

It is not meant to be directly inherited by user-defined classes (for that, use Exception).

@alexcjohnson
Copy link
Collaborator

Actually StopIteration does inherit from Exception - but to your point @0zeroth GeneratorExit does not (nor do SystemExit or KeyboardInterrupt - see https://julien.danjou.info/python-exceptions-guide/) and the reason given is essentially what you said, "since it is technically not an error."

However, thinking about it a bit more, I suspect it would actually be more confusing than useful to have PreventDefault not be an Exception. PreventDefault will generally be explicitly raised (unlike the other BaseException subclasses) so it would be surprising to not be able to catch it with a "normal" except Exception as e:. I notice however that a bare except: with no class at all does catch BaseException... making this even more convoluted. Users following the conventional wisdom to catch only the specific exception classes they expect won't care one way or the other.

So, all that said, unless we have a clear use case for BaseException I'd say let's keep it an Exception.

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

No branches or pull requests

3 participants