Add mypy type hints for utils #862
Conversation
|
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 |
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 |
|
Likely should remain a subclass of Also is the type checker fine without those |
|
Agreed, should confirm with Miruna before making this change |
|
Marked back as In Progress until we resolve the above discussions |
|
Hi All, apologies for the delay in replying. Before I jump back in the discussion, let me just preface that I'm not an expert of the utils folder, and this is basically coming from discussions with copilot and some reading of the mypy docs. Please let me know if I've misunderstood something >.< . NoReturnFrom my understanding, noreturn means the function should always raise or exit. When we have a function that does nothing, it basically does an implicit 'return None', which is why mypy complains with this
Thats why I swapped the NoReturns to the Nones. There might be a better way? Can be disabled with Was
|
|
Re Typechecking, this is a reminder to myself to talk to Pawel about how type_checking is done with stubs in protobuf etc |
|
While not necessary for the class to work, we'd want to keep the class inheritance in |
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