units: allow return type annotations to contain non-unit types#11506
Conversation
|
Thanks! Given this is a bug, will need a change log too. We have changed the change log format, see https://github.com/astropy/astropy/blob/main/docs/changes/README.rst |
|
I am not convinced this needs a backport but reviewers will change the milestone if they disagree. |
mhvk
left a comment
There was a problem hiding this comment.
Thanks! This is easy to review and I now definitely agree we may as well fix this, even though the larger reorganization by @nstarman may change it slightly. (@nstarman - your work would not change the outcome of the test, correct?)
Since all I have is a nitpick, I'll approve (but leaving it open for a little while for possible other comments)
| if wrapped_signature.return_annotation not in valid_empty: | ||
| if (wrapped_signature.return_annotation not in valid_empty) and isinstance( | ||
| wrapped_signature.return_annotation, (str, UnitBase, FunctionUnitBase) | ||
| ): |
There was a problem hiding this comment.
Total nitpick, but could the closing parenthesis be on the previous line? I still find this aspect of blacken really unpythonic and cluttery... (Also, beyond PEP8 our general request is to follow the style of existing code in a file)
There was a problem hiding this comment.
Instead of any string, the code should check for a valid physical type. mypy and similar allow for a string annotation to be used instead of a class, where the class has not yet been defined.
There was a problem hiding this comment.
Ideally, yes, but I think here I think it is fine to be simple.
But, now thinking about it: since the return annotation should normally be a class if it is not a unit, could one just do not isinstance(..., type)?
There was a problem hiding this comment.
There was a problem hiding this comment.
This won't be a problem starting in python 3.10, but it can be problematic here.
There was a problem hiding this comment.
It should be good enough 👍
def func(x) -> int:
pass
isinstance(func.__annotations__["return"], type)
>>> True
There was a problem hiding this comment.
Wait! further tests are problematic
There was a problem hiding this comment.
import typing
class A:
pass
def func(x) -> typing.Type[A]:
return A
isinstance(func.__annotations__["return"], type)
>>> False
There was a problem hiding this comment.
The current implementation appears the safest.
There was a problem hiding this comment.
OK, @noc0lour, let's just stick with what you have. Could you still change the parenthesis back to the previous line? You can add [skip ci] to the commit to avoid rerunning all the tests. Happy to merge with that!
|
Oops, just noticing @pllim's comment - yes, please do add a changelog fragment as well! This is a nicely solves something that has bugged people before, after all! |
|
Alright, I'll change the parenthesis & add the changelog in a bit. Thanks for the quick review :) |
This commit tests the type of the return type annotation and only performs conversion if the type is either `str` or a Unit type. This puts the return type annotations in line with other annotations. [skip ci]
680188a to
fe6a47c
Compare
mhvk
left a comment
There was a problem hiding this comment.
Thanks, looks all OK now, so will approve and merge...
Description
This pull request is provides a small, backportable bugfix to allow non-unit types to be used as return type annotations.
These non-unit types are then ignored by
@quantity_inputdecorators, such that no conversion is attempted.Fixes #10156