Skip to content
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

ci: add type checks #88

Merged
merged 6 commits into from
Jan 23, 2024
Merged

ci: add type checks #88

merged 6 commits into from
Jan 23, 2024

Conversation

JaeAeich
Copy link
Contributor

IMPORTANT: Please create an issue before filing a pull request! Changes need to be discussed before proceeding. Please read the contribution guidelines.

Details
This PR add type checking using mypy as stated in issue #77

Closing issues

Fixes issue - #77

@JaeAeich JaeAeich mentioned this pull request Jan 10, 2024
@JaeAeich
Copy link
Contributor Author

@uniqueg Is this good (in right dir)? I still need to narrow it down a little as I have ignored import errors in mypy.ini which I think needs a little more work.

Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JaeAeich. Quite a number of comments, but I guess they are mostly small things (except perhaps the .wes_endpoint validation, which, however, is not necessary).

.github/workflows/main.yml Outdated Show resolved Hide resolved
mypy.ini Show resolved Hide resolved
pro_wes/app.py Outdated Show resolved Hide resolved
pro_wes/app.py Outdated Show resolved Hide resolved
pro_wes/app.py Outdated Show resolved Hide resolved
requirements_dev.txt Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
pro_wes/tasks/track_run_progress.py Outdated Show resolved Hide resolved
pro_wes/tasks/track_run_progress.py Outdated Show resolved Hide resolved
pro_wes/tasks/track_run_progress.py Outdated Show resolved Hide resolved
@JaeAeich
Copy link
Contributor Author

JaeAeich commented Jan 18, 2024

Hey @uniqueg, unfortunately mypy doesn't check that deep, this is in regard to your suggestion to create a validator func. So the other solution would be something like this:

        assert document_stored.wes_endpoint is not None, "No WES endpoint available."
        assert (
            document_stored.wes_endpoint.base_path is not None
        ), "WES endpoint does not have base_path."

        if document_stored.task_id is None:
            raise WesEndpointProblem

Where WesEndpointProblem is

....
class WesEndpointProblem(NotFound):
    """No/few reuirements provided for WES endpoint."""


exceptions={
   ....
    WesEndpointProblem: {
        "message": "No/few reuirements provided for WES endpoint.",
        "code": "500",
    }
}

yeah that mean I gotta do it in two place ie, for document, updated_document and in track_run_progress. Please do tell me if there is better way to do it.
PS: Not yet ready for reviews.

Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! Just one question I still have (but maybe I'm missing something)

pro_wes/ga4gh/wes/workflow_runs.py Outdated Show resolved Hide resolved
@uniqueg
Copy link
Member

uniqueg commented Jan 22, 2024

Hey @uniqueg, unfortunately mypy doesn't check that deep, this is in regard to your suggestion to create a validator func. So the other solution would be something like this:

        assert document_stored.wes_endpoint is not None, "No WES endpoint available."
        assert (
            document_stored.wes_endpoint.base_path is not None
        ), "WES endpoint does not have base_path."

        if document_stored.task_id is None:
            raise WesEndpointProblem

Where WesEndpointProblem is

....
class WesEndpointProblem(NotFound):
    """No/few reuirements provided for WES endpoint."""


exceptions={
   ....
    WesEndpointProblem: {
        "message": "No/few reuirements provided for WES endpoint.",
        "code": "500",
    }
}

yeah that mean I gotta do it in two place ie, for document, updated_document and in track_run_progress. Please do tell me if there is better way to do it. PS: Not yet ready for reviews.

Looks fine to me, not too messy :)

@uniqueg
Copy link
Member

uniqueg commented Jan 22, 2024

Sorry, only saw now that you were not ready for review. Gave you one anyway 😆

But it seems to me that you addressed at least most of my previous issues. Please make sure to resolve the corresponding conversations so that we can better keep track of what's still left to do.

@JaeAeich
Copy link
Contributor Author

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
foca 0.12.1 requires pymongo==3.10.1, but you have pymongo 3.13.0 which is incompatible.

I am disabling all error for missing imports, when foca update pymongo to 3.11 we can remove it. This is because
pymongo-stubs is compatible with Python >=3.6 and [PyMongo](https://pypi.org/project/pymongo/) >=3.11,<4.0.

@JaeAeich JaeAeich requested a review from uniqueg January 22, 2024 14:38
@JaeAeich
Copy link
Contributor Author

@uniqueg Please review the PR. I have made the requested changes 👍

Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great - thanks a lot @JaeAeich :)

@uniqueg uniqueg changed the title add: type checking style: type checking Jan 22, 2024
@uniqueg uniqueg changed the title style: type checking ci: add type checks Jan 22, 2024
@JaeAeich JaeAeich merged commit f5bdff7 into dev Jan 23, 2024
3 checks passed
@JaeAeich JaeAeich deleted the type-checking branch January 23, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants