Skip to content

Conversation

@delicb
Copy link
Contributor

@delicb delicb commented Oct 6, 2021

Not really sure what was wrong with the original code and why it did not work, but I could reproduce it consistently. This change simplifies typing.py and it passes my local tests. I have tried mypy with following combinations (results in the comments):

from werkzeug.exceptions import HTTPException

class MyHTTPException(HTTPException): pass


# PASS - base case
@app.errorhandler(Exception)
def handle_any_exception(e: Exception) -> tuple[str, int]:
    return "generic error", 500


# PASS - non-base exception, using werkzeug.exceptions.HTTPExceptions as example
@app.errorhandler(HTTPException)
def handle_http_exception(e: HTTPException) -> ResponseReturnValue:
    log.exception("unknown error", exc_info=e)
    return "ERROR", e.code


# PASS - handling more specific exception, accepting more general one as parameter, should work
@app.errorhandler(HTTPException)
def handle_http_exception_lower_type(e: Exception) -> ResponseReturnValue:
    return str(e), 500


# PASS - custom defined exception in error handler, base exception as parameter
@app.errorhandler(MyHTTPException)
def handle_my_http_exception(e: HTTPException) -> ResponseReturnValue:
    return "error", e.code


# FAIL - since handler is accepting broader exception than declared in errorhandler parameter
@app.errorhandler(Exception)
def handle_any_exception_wrong_parameter(e: HTTPException) -> ResponseReturnValue:
    return str(e), e.code


# FAIL - unrelated exceptions
@app.errorhandler(KeyError)
def handler_value_error(e: ValueError) -> tuple[str, int]:
    return "value error", 500


# PASS - works as before, no changes
@app.errorhandler(404)
def handle_404(error: Exception) -> tuple[str, int]:
    return "not found", 404

Equivalent calls to app.register_error_handler behave the same as a decorator usage, as expected.

I did not find any tests of framework to tests typing, that is why I performed manually local testing. If I should change the code to test this change, please do let me know.

Please let me know if I should have done something different regarding CHANGES.rst.
Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@delicb delicb changed the base branch from main to 2.0.x October 6, 2021 23:25
@delicb
Copy link
Contributor Author

delicb commented Oct 6, 2021

I see no conflicts between this patch and 2.0.x branch. It seems that mergable check got stuck because I made a mistake and sent PR to main instead of 2.0.x branch initially.

@davidism davidism closed this Oct 7, 2021
@davidism
Copy link
Member

davidism commented Oct 7, 2021

Sorry for the issue, please open a new PR.

@delicb
Copy link
Contributor Author

delicb commented Oct 7, 2021

No problem. Done: #4298

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

error handler type check fails

2 participants