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

remove NoReturn, use None instead #1320

Merged
merged 1 commit into from
Aug 24, 2019
Merged

Conversation

toppk
Copy link
Contributor

@toppk toppk commented Aug 23, 2019

  • Added tests for changed code.
  • Updated documentation for changed code.

I don't think the use of NoReturn is correct. These functions return, just with None

pinging original author @abn of the original feature.

#210
#774

For reference, this breaks pypy compatibility, which has an older version of typing.

@brycedrennan brycedrennan added the area/docs Documentation issues/improvements label Aug 24, 2019
@brycedrennan
Copy link
Contributor

@toppk According to this explanation you are correct. I'm not super familiar with python typing: aren't these just comments? how does this break compatibility?

@dmontagu
Copy link

dmontagu commented Aug 24, 2019

The NoReturn is being imported, which will cause an error if not present in the implementation’s typing module.

I think the idea that they are comments might stem from the fact that if you include from __future__ import annotations, the annotations are all strings are not evaluated. But even then, if you include typing-related imports, they will be executed just like any other python import.

Given the function isn’t actually a NoReturn (which should only be used if the function is guaranteed to throw an exception under all circumstances, or otherwise fail to return) it seems to make sense to change this, especially given that it is causing compatibility issues.

Edit: I looked and realized it is actually only used in comments. The problem is for the type checker (presumably mypy) to recognize it, it needs to be imported, which is the source of the incompatibility.

Also, the official docs on this: https://mypy.readthedocs.io/en/latest/more_types.html

@abn
Copy link
Member

abn commented Aug 24, 2019

@toppk good catch; over looked that one. 👍

@abn
Copy link
Member

abn commented Aug 24, 2019

@brycedrennan can we get this merged please?

@brycedrennan brycedrennan merged commit 4d87bba into python-poetry:develop Aug 24, 2019
@Caligatio Caligatio mentioned this pull request Aug 30, 2019
2 tasks
Copy link

github-actions bot commented Mar 1, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/docs Documentation issues/improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants