-
-
Notifications
You must be signed in to change notification settings - Fork 94
Configure stubtest hook for stub testing #415
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
Conversation
This comment has been minimized.
This comment has been minimized.
@webknjaz why does the CI fail? |
Because of #415 (comment) (plus the commit check fails because of the commit message format). |
Okay, I configured now the setting and improved the commit message. |
Yeah, that one is broken. We need to report a bug to the action upstream. But https://github.com/cherrypy/cheroot/runs/4780587437?check_suite_focus=true is a legitimate failure that should be fixed. |
Can you please do that? Also codeclimate complains, but the page is a 404 error... |
Sure, I can fix it. I think that it probably just requires some extra deps with types.
Oh, that is weird. It's 404 for me too. Will have to look into it separately. |
@kasium I've rebased this on top of the master with fixed CI and figured out how to reuse the present mypy repo definition and call |
@webknjaz to be honest I don't work with pre-commit in my projects, so I can't analyse this in detail. If you run stubtest with the arguments directly it works fine... What I figured out is, that for whatever reasons, the
However, then this error shows up:
I have no clue why this happens, but I guess this is related to mypy. stubtest works fine. Not sure, but MAYBE it's way easier to move the pyi files to typeshed and then to add inline type hints when you decide to drop python2? |
@kasium thanks for looking into it. I just realized that args values like (This is because if I'll try to see what can be done on this level and report back. |
Hacking it to use |
I think adding places to watch over is going to increase the synchronization+maintanance burden. So I'd rather stick with the current approach. |
@kasium I think I've made the pre-commit+tox setup correct now. But I'm seeing some errors that seem to be legitimate. |
@webknjaz I fixed the issues (see also comments in the ignore file) |
|
||
ssl_conn_type: SSL.Connection | ||
ssl_conn_type: Type[SSL.Connection] |
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.
Wonder if this could be made optional (because it is in runtime).
This hook is not declared officially so this addition piggy-backs on what the MyPy repository provides redefining the invocations. It checks if the ``.pyi`` files match the ``.py`` files. Part of #384
@kasium thanks for fixing that! I've also fixed the dependencies (because the current pyi definition assumes that pyOpenSSL is always present in the env while in fact, it's optional in runtime): MyPy stubtest................................................................Failed
- hook id: stubtest
- exit code: 1
error: cheroot.ssl.pyopenssl.ssl_conn_type is not present at runtime
Stub: at line 6
Type[OpenSSL.SSL.Connection]
Runtime:
MISSING |
Cool, thanks! |
β What kind of change does this PR introduce?
π What is the related issue number (starting with
#
)Part of #384
β What is the current behavior? (You can also link to an open issue here)
No stub testing.
β What is the new behavior (if this is a feature change)?
This pre-commit hook (which doesn't seem to exist yet, therefore we need a local one) will check if the .pyi files matching the .py files. With this, I already found several issues which are fixed in this commit as well
π Contribution checklist:
(If you're a first-timer, check out
this guide on making great pull requests)
the changes have been approved
and description in grammatically correct, complete sentences
This change isβ