Add python typechecking for mypy on src/ci_workflow and tests/tests_ci_workflow #1582
Conversation
Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Zelin Hao <[email protected]>
|
What's left after this one to add typechecking to the entire python codebase? |
|
We are tracking with meta issue, #1237, ~3 more PRs to go |
|
|
||
|
|
||
| class CiCheck(ABC): | ||
| def __init__(self, component, target, args=None): | ||
| def __init__(self, component: Any, target: CiTarget, args: Any = None) -> None: |
There was a problem hiding this comment.
| def __init__(self, component: Any, target: CiTarget, args: Any = None) -> None: | |
| def __init__(self, component: Component, target: CiTarget, args: Any = None) -> None: |
Seems like component should be a Component type instead of any
| @@ -23,6 +28,6 @@ class CiCheckDist(CiCheck): | |||
|
|
|||
|
|
|||
| class CiCheckSource(CiCheck): | |||
| def __init__(self, component, git_repo, target, args=None): | |||
| def __init__(self, component: Component, git_repo: GitRepository, target: CiTarget, args: Any = None) -> None: | |||
There was a problem hiding this comment.
Could we make args more specific?
There was a problem hiding this comment.
I was using CiArgs type for this args, however, for example CiCheckGradleDependencies inherits from this CiCheckSource isn't necessarily taking CiArgs input; it was just a simple string. This is the test case we have
| @@ -6,18 +6,22 @@ | |||
|
|
|||
| import logging | |||
| import re | |||
| from typing import Any | |||
There was a problem hiding this comment.
I see 14 imports of Any, let's try to get this to zero or have an explanation for it. Even an untyped Dict/List is better than nothing.
There was a problem hiding this comment.
Note; I'm OK with this as a follow up issue.
|
|
||
|
|
||
| class CiCheckList(ABC): | ||
| def __init__(self, component, target): | ||
| def __init__(self, component: Any, target: CiTarget) -> None: |
There was a problem hiding this comment.
Can we do component: Component?
| from manifests.input_manifest import InputComponentFromDist, InputComponentFromSource | ||
|
|
||
|
|
||
| class CiCheckLists(ABC): | ||
| @classmethod | ||
| def from_component(self, component, target): | ||
| def from_component(self, component: Any, target: CiTarget) -> Any: |
| def __init__(self, manifest, args): | ||
|
|
||
| class CiManifest(ABC): | ||
| def __init__(self, manifest: Any, args: CiArgs) -> None: |
There was a problem hiding this comment.
Seems like this should be a manifest type, maybe an InputManifest?
|
I'll be merging this now to unblock the process and will create a separate PR for any followup issues Thanks. |
…i_workflow (opensearch-project#1582) * Add python typechecking for ci-workflo Signed-off-by: Zelin Hao <[email protected]>
Description
pipenv run mypy src/ci_workflow tests/tests_ci_workflowruns without errorIssues Resolved
#1508
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.