| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
| if annotation is not sig.empty and annotation != inspect._empty: | ||
| return inspect.formatannotation(annotation).replace("'", "") |
There was a problem hiding this comment.
This seems odd to me, for various reasons:
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, that inspect._empty condition is actually redundant. I investigated the logic further and there were lots of inconsistent behaviors in the test (that's why I previously did replace("'", "")) and corner cases. However, the new solution (though a bit hacky) is the most general one and hopefully covering everything.
Sorry, something went wrong.
| @@ -0,0 +1 @@ | |||
| Added return type annotations in ``fixtures`` and ``fixtures-per-test``. | |||
There was a problem hiding this comment.
| Added return type annotations in ``fixtures`` and ``fixtures-per-test``. | |
| Added return type annotations in ``--fixtures`` and ``--fixtures-per-test``. |
Sorry, something went wrong.
| if annotation.__module__ == "typing": | ||
| return str(annotation).replace("typing.", "") | ||
| return annotation.__name__ | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
Why would ValueError and TypeError happen here? The only thing that could happen I think is AttributeError, which already happens with e.g. -> None: as annotation.
That being said, None should definitively handled correctly by the code and tested properly as well.
Sorry, something went wrong.
There was a problem hiding this comment.
TypeError happens when the argument is not a callable (e.g. a number). I doubt this case happens. ValueError happens when the passed argument is a callable but a signature can't be obtained (e.g. range). Also unlikely imo.
For -> None: on my local test didn't raise any error.
Sorry, something went wrong.
There was a problem hiding this comment.
Both of those cause neither of those exceptions, but cause an AttributeError for .__module__.
Sorry, something went wrong.
| result.assert_outcomes(passed=1) | ||
|
|
||
|
|
||
| def test_get_return_annotation() -> None: |
There was a problem hiding this comment.
Maybe turn this into a test class and split the individual asserts into separate test functions? That way, if one of them fails, the rest of the tests will still run.
Sorry, something went wrong.
|
|
||
| assert get_return_annotation(no_annotation) == "" | ||
|
|
||
| def none_return() -> None: |
There was a problem hiding this comment.
Ah, this test happens to work (despite the bug above) because this module uses from __future__ import annotations, so the annotation already is 'None' rather than None.
Not sure how to best get around this. Maybe those tests should be in a separate module which deliberately doesn't use that?
Sorry, something went wrong.
There was a problem hiding this comment.
On my local test (3.13) with or without from __future__ import annotations it worked the same. I might be very wrong but I think it behaves differently in different Python versions. Still, I added a exclusive check for none. Do you think moving it to another module is worth it? Seems too much for a humble helper function!
PS: The reason I didn't use `isinstance(annotation, types.NoneType) was that pylance was complaining.
Sorry, something went wrong.
|
@The-Compiler Could you please review again? |
Sorry, something went wrong.
|
Sorry for the radio silence, I never got around to taking a deeper look at this, seems like getting a type annotation as a string is surprisingly difficult with a lot of corner cases... I still think having a file without the __future__ import is absolutely crucial for those tests - because as-is, they don't actually test most of your code, and that shows in coverage. Trying with -> tuple[int, str] in a new file, all that's displayed is tuple, and I'm pretty sure after looking at the implementation of inspect.formatannotation that there are more funny corner cases that possibly cause a crash currently (e.g. -> 42 currently does - while not a valid annotation semantically, it is valid Python). Given the amount of corner cases, I wonder if (despite what I said earlier) we should go back at letting inspectlib handle those. Perhaps we can lessen the future impact of using an undocumented function by passing annotation_format=annotationlib.Format.STRING to inspect.signature with Python >= 3.14, then we can take an easy shortcut there and simply return the string directly in that case. #13550 added a helper signature method that does this already, so I'd recommend basing things on that. |
Sorry, something went wrong.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Are you suggesting to use formatannotation again? |
Sorry, something went wrong.
|
At least for Python < 3.14, for two reasons:
|
Sorry, something went wrong.
|
In addition to the current code, I've tried inspect.get_annotation (with or without formatannotation) and typing.get_type_hints and even with lots of tinkering they all fail miserably on the test cases. It seems impossible! There's another approach, parsing with ast, which works decently, however, the tests fail because it can't access the source code. Any suggestions @The-Compiler ? |
Sorry, something went wrong.
|
I suspected this is going to be trickier than it looked on the surface 😅. I really hope we don't need to resort to AST shenanigans, after all, other tools do sometimes print type annotations as well I think? Can you elaborate on "all fail miserably on the test cases"? I'd hope that inspect.signature (or inspect.get_annotations which is internally used by inspect.signature) and formatannotation (+ forcing a string on Python 3.14+) should do the right thing here, but it sounds like that's not the case? |
Sorry, something went wrong.
I mean the test cases fail. For example doing: try:
annotation = inspect.get_annotations(fixture_func)["return"]
return inspect.formatannotation(annotation)
except (KeyError, ValueError, TypeError):
pass
return ""
will give collections.abc.Callable[..., typing.Any] instead of Callable[..., Any] in the test case. The more surprising thing is that when I run the code in IPython for testing, it produces the desired output Callable[..., Any]. |
Sorry, something went wrong.
|
I think that's fine, after all that's what inspect.signature does as well: import inspect
from collections.abc import Callable
def func(x: Callable[[], None]) -> None:
pass
print(inspect.signature(func))And with the approach outlined above, starting on Python 3.14 (or before with from __future__ import annotations) it will print whatever is there in the source file anyways, so I wouldn't worry about it too much. |
Sorry, something went wrong.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
The best that I could come up with was treating signature as string (as you suggested) and use regex to remove clutterings (if present) and it passed the tests locally. But again the tests are failing in CI and codecov is complaining unfairly. Any tips? |
Sorry, something went wrong.
|
That's not what I meant with treating it as a string at all 😅 Parsing the string output definitely isn't necessary here, as you're just picking out what was rendered using formatannotation. Despite the caveat of that being undocumented, calling it is definitely a lot better than parsing things from the string. What I meant was that with Python 3.14, you can pass annotation_format=annotationlib.Format.STRING to inspect.signature (or probably better to use annotationlib.get_annotations() at this point, as you need to import it anyways and we don't need anything from inspect). Then from my understanding, you always get a string back, and you don't need to format it any way. So basically I see three scenarios the code needs to handle:
As for the coverage, looks like pre-commit tried to be helpful and introduced the __future__ import again: 1c18e59 This seems to happen via ruff: Line 102 in a55c959 So if you add a # ruff: noqa: FA100 to the top of that file, that should hopefully stop happening (docs). |
Sorry, something went wrong.
|
Understood, but formatannotation doesn't work well, for example it returns: |
Sorry, something went wrong.
|
That's because it formats the __module__ and the __qualname__ into the output. Maybe that could actually be useful information in real-world scenarios (try e.g. with class T being defined at module level instead of inside a function), but yeah, it could also be too verbose. It looks like formatannotation also supports a base_module you can pass to avoid including that part if the class is defined in the same module the fixture is (by passing the fixture module as base). If you disagree, I suppose we're back to copying the formatannotation code, and adjusting it so that it uses annotation.__name__ in case the annotation is a type (i.e. a class). Would be fine in my book as well, as it's only some 10 lines. |
Sorry, something went wrong.
for more information, see https://pre-commit.ci
I've tried it but it doesn't work as intended. Do you object using regex to remove the extra verbose parts?
|
Sorry, something went wrong.
|
@kianelbo this has gotten stale. Would you mind rebasing? Tip Pro tip: in the future, it's best to file PRs from feature branches. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for getting this started
This looks readable
Why was a new file created and was an ai assistant involved
Sorry, something went wrong.
| def get_return_annotation(fixture_func: Callable[..., Any]) -> str: | ||
| pattern = re.compile(r"\b(?:(?:<[^>]+>|[A-Za-z_]\w*)\.)+([A-Za-z_]\w*)\b") | ||
|
|
||
| sig = signature(fixture_func) |
There was a problem hiding this comment.
We need to avoid resolving unresolvable type annotations there
This is an issue with python 3.14 on certain codebases
Sorry, something went wrong.
There was a problem hiding this comment.
Do you mean sometimes signature will raise an error, depending on the case? Could you provide an example, so we can add a test for it?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes this also is a issue in pluggy but there is can sidestep the issue differently
Sorry, something went wrong.
|
|
||
|
|
||
| def get_return_annotation(fixture_func: Callable[..., Any]) -> str: | ||
| pattern = re.compile(r"\b(?:(?:<[^>]+>|[A-Za-z_]\w*)\.)+([A-Za-z_]\w*)\b") |
There was a problem hiding this comment.
This regex needs a explanation and if we compile it we should do that either once or cached
Sorry, something went wrong.
|
OK I'm done with github mobile its ui glitches get me every time |
Sorry, something went wrong.
Btw I'm OK with just displaying what formatannotation returns, without any regex matching. Might be a bit cluttered, but keeps the code simple and we don't hide possible useful information to the user. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
closes #13676