Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add typing for function_decorator #23
feat: Add typing for function_decorator #23
Changes from 11 commits
2ea3e9c
4c31ae5
7c1ca25
08ecfd4
d0e97bf
13d8e03
bbf24fe
e4ddeab
a13877f
d5762b5
69ff648
ad3e2f3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should do the same for
class_decorator
and fordecorator
, don't you think ?Note that
_Decorator
might need to change to support decoratingType
s too. Or, even more ideally, we would have a_FuncDecorator
forfunction_decorator
, a_ClassDecorator
forclass_decorator
and a_Decorator = Union[_FuncDecorator, _ClassDecorator]
fordecorator
.Would it make sense ?
Note: this may be for another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense. I could try to do it in next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just being curious here: isn't there a way to assert typing-related things in pytest ? We can even consider using
typing-inspect
or another such package if it makes sense..Alternately we could add a
mypy
step in the nox file, scanning precisely this test file ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember few days ago seeing article about testing typing in python packages, but couldn't find it at the moment.
We cannot just run
mypy
orpyright
in CI, becouse it will exit with errors, and we want to be sure that it fails where we expect it to fail.I think, our best shot is to run
mypy
andpyright
using subprocess inpytest
, and compare it's output with what we're expecting. I could try to do this in next PR or in this, what do you think?But before that i want to think a little about other options, and maybe see what others are doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for
typing-inspect
, i don't think it could be used in this case. Sure it could check something, but i'd want to test exact cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, i found something.
django-types are running
pyright
in subprocess, and parsing it's messageshttps://github.com/sbdchd/django-types/blob/main/tests/pyright/base.py
and, mypy could be called from pytest using it's api:
https://mypy.readthedocs.io/en/stable/extending_mypy.html
I tried using mypy, but strumbled upon a bug which crashes mypy. Now i'm trying to fix it or make minimal example to report bug in mypy.
I think, merging this could actually crash mypy for someone who uses it.
python/mypy#8645
They already have similar issues with other code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nevermind. It was crashing becouse it has cached some data from previous version in my environment. It's working fine. Just doesn't supports those ParamSpecs properly yet.
So, i think, if we want to add type testing, we could use pyright just like django-types. (But maybe that's for another PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great ! Ok, let's keep this for another PR.