-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add type annotations to function signatures in credential_plugins_test and importable_test modules #26
Conversation
Judging by the RTD failures, the branch needs to be rebased (and some CI jobs will become green as a result). |
@djyasin here's the current state of MyPy coverage: https://app.codecov.io/gh/ansible/awx-plugins?flags%5B0%5D=MyPy. You can look into that or check what's in |
98e015d
to
2d542a7
Compare
This PR contains all of the fixes for |
This problem https://github.com/ansible/awx-plugins/actions/runs/10740379607/job/29788419764?pr=26#step:18:29 shows that there is a dependency on src/awx_plugins/credentials/plugins.py:7:1: error: Cannot find implementation
or library stub for module named "awx.main.models.credential"
[import-not-found] Though, we cannot physically declare it due to it not being on PyPI (well, there's a wrong The way to do it is by reproducing a section like https://github.com/sphinx-contrib/sphinxcontrib-towncrier/blob/b01f3b0/.mypy.ini#L36-L40 and configuring just that section to pretend there are no such imports... |
5019634
to
0bb608f
Compare
7696c90
to
2f3e80d
Compare
FYI, this failure https://github.com/ansible/awx-plugins/actions/runs/10818045643/job/30012894850?pr=26#step:18:49 is flaky. I reported it upstream via sphinx-contrib/spelling#227. For now, just restart the job if you see this error. |
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.
Let's move the unrelated fixes elsewhere so that this PR won't have to accumulate more changes required for those unrelated fixes to be complete.
@djyasin I just realized that the PR description and title need to be updated to reflect what's fixed. And also to be accurate... |
683df2a
to
37b4d90
Compare
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.
There's a few places that need to be addressed still.
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.
@djyasin feel free to squash the commits and put this into the merge queue.
@djyasin I think the PR title needs to be clarified still. It's pretty generic in describing what is done. It would be nice to see what exactly is being changed. Also, some MyPy violations going away is one of the effects of the patch. The title should be talking about the change that leads to those effects. Like adding typing. Or rather adding type annotations to function signatures in these modules. Not sure if this covers all violations in these modules (probably not?) |
…table_test.py Update tests/credential_plugins_test.py Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]> Add type annotation to line 19
a30e167
to
a9c9702
Compare
This PR contains the type annotations to function signatures for credential_plugins_test.py and tests/importable_test.py