Conversation
There was a problem hiding this comment.
There was a problem hiding this comment.
Hi James, ah yes thanks thats useful. Didn't know about typeshed so will have a go through with that.
Yeah I was wondering if it were better to have the mypy stuff in a separate file or combined the toml. After thinking about it I think ur right, keeps everything centralised
There was a problem hiding this comment.
Thank you for this discussion. For the linitng, I would suggest we leave that in a separate PR so that linting does not otherwise fail while we work on the mypy development. Having a configurattion in pyproject.toml would be better, but that can also be for a separate PR that closes #828
As for typeshed, I have left that as a suggestion in #828 as well. I think it would be very useful
|
Note: when developing this it was discovered that ruff has two settings in drunc, one in .ruff.toml and another in the pyproject.toml. We should probably choose which one we want to keep, and also probably propagate to mypy. Basically
To be discussed |
|
This is great work, thanks @emmuhamm . I left some comments. Once they are addressed I will review, but this looks like the exact changes we need towards working with
These should be consolidated, if it can be done with the pre-commit workflows. I would expect that this can be done. The |
There was a problem hiding this comment.
Thank you for this discussion. For the linitng, I would suggest we leave that in a separate PR so that linting does not otherwise fail while we work on the mypy development. Having a configurattion in pyproject.toml would be better, but that can also be for a separate PR that closes #828
As for typeshed, I have left that as a suggestion in #828 as well. I think it would be very useful
| prod = ["paramiko[gssapi]"] | ||
| dev = ["ruff", "pre-commit", "pytest", "pytest-cov", "grpcio-testing", "grpcio==1.75", "grpcio-tools==1.75", "grpcio-status==1.75"] | ||
| test = ["pytest", "pytest-cov", "grpcio-testing", "grpcio==1.75", "grpcio-tools==1.75", "grpcio-status==1.75"] | ||
| test = ["pytest", "pytest-cov", "grpcio-testing", "grpcio==1.75", "grpcio-tools==1.75", "grpcio-status==1.75", "mypy", "types-requests"] |
There was a problem hiding this comment.
Note that as per the mypy.ini comment, we will leave these until a dedicated PR for #828 gets created. In that PR, we will also need to add these to the dev settings so at commit times we can have these checks. May be worth adding something to the pre-commit config too.
This reverts commit 0ddb530b528403f637df530f6d578b818bc4ac2b.
015d0ad to
2981ad4
Compare
|
Hi @PawelPlesniak this is now ready for review. I've updated the description so hopefully thats all clear. lmk if somethings not clear :) |
|
Note no changes were introduced from the merge of develop in the |
|
Nice work on this. Integ tests are currently running. 2 questions
|
|
Manual tests passed |
Description
Fixes issue #852
Update utils folder to follow mypy standards
This is the first step in introducing mypy to all of drunc. This PR only concerns itself with the utils folder.
The mypy and ruff settings that were used is in this commit: 2097249
It is highly recommended that those who want to add mypy to drunc also follows these settings.
Explanation of the ruff and mymy settings
Meaning of the ruff codes
D + convention = "google" tells ruff to check docstrings against Google style
Meaning of the mypy codes
General ones
Any-specific rules
It should be noted that we avoid changing the TOMLs in this PR. Therefore, if you run mypy right now, then these are the errors you might encounter
Details
With no changes in the toml, the output is as followsType of change
List of required branches from other repositories
N/A
Suggested manual testing checklist
To check if its safe to merge in develop
The usual
To check if the mypy checks work
pip install -e .[dev,test]mypy src/drunc/utils/. All checks should passruff check src/drunc/utils/. All checks should passDeveloper checklist
Prior to marking this as "Ready for Review"
Tests ran on: np04-srv-028 from release Nightly from 16 April.
Unit tests - some tests can't be ran on the CI. This is documented. If this PR checks a feature that can't be tested with CI, this has been marked appropriately.
Integration tests - the
daqsystemtest_integtest_bundlerequires a lot of resources, and connections to the EHN1 infrastructure. Check the cross referenced list if you can't run these. The developer needs to run at least the .pytest --marker) passeddaqsystemtest_integtest_bundle.sh -k minimal_system_quick_test.pydaqsystemtest_integtest_bundle.shFinal checklist prior to marking this as "Ready for Review"
Reviewer checklist
druncare in the log filesdruncfailure appears:Once the features are validated and both the unit and integration tests pass, the PRs is ready to be merged.
Choose one of the following an complete all substepsPrior to merging
Once completed, the reviewer can merge the PR.
Notification message for #daq-sw-librarians Slack channel
For an single merge that changes the user workflow
For co-ordinated merge