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

Mark abort as having type NoReturn #2020

Merged
merged 1 commit into from
Jan 26, 2021
Merged

Conversation

bjgill
Copy link
Contributor

@bjgill bjgill commented Jan 26, 2021

In #1995, typing was added to large parts of this project. This will be really useful to me.

One of the problems I have with the latest release is abort. PyCharm doesn't know that abort aborts. So it will complain about possibly uninitialised variables if I write code like this:

if foo:
    bar = 1
else:
    abort(404)

print(bar)

This is because abort is declared to return type None: def abort(status: t.Union[int, "Response"], *args, **kwargs) -> None:. This is not the case - I think abort always raises an exception.

Instead, what we want to do is change the return type to t.NoReturn. This expresses the correct meaning, which is that the abort function never returns. In turn, this will help PyCharm and other analysers understand code that uses abort better.

Checklist:

(a docs change, so I've skipped some steps)

- [ ] 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. - this PR builds on redo type annotations #1995, which doesn't have an entry. Should I add an entry to cover both, or leave it alone?
    - [ ] Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@davidism
Copy link
Member

See the test failure for why I didn't do this.

In pallets#1995, typing was added to large parts of this project. This will be really useful to me.

One of the problems I have with the latest release is `abort`. PyCharm doesn't know that `abort` aborts. So it will complain about possibly uninitialised variables if I write code like this:

```
if foo:
    bar = 1
else:
    abort(404)

print(bar)
```

In master, `abort` now returns type `None`: `def abort(status: t.Union[int, "Response"], *args, **kwargs) -> None:`

This type annotation means that the function returns `None`. This is not the case - I think `abort` always raises an exception.

Instead, what we want to do is change the return type to [`t.NoReturn`](https://www.python.org/dev/peps/pep-0484/#the-noreturn-type). This expresses the correct meaning, which is that the `abort` function never returns. In turn, this will help PyCharm and other analysers understand code that uses `abort` better.
@bjgill
Copy link
Contributor Author

bjgill commented Jan 26, 2021

Good point - I missed this in the tox output. I think I've found the problem - mypy's type inference doesn't seem to understand _aborter. It looks as if being explicit about the type of _aborter fixes the problem, though.

I'm not entirely sure why this is, which is a bit unsatisfactory.

@davidism davidism merged commit b9f9af5 into pallets:master Jan 26, 2021
@davidism
Copy link
Member

Thanks for figuring it out!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 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.

2 participants