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

Pre-commit: exclude mypy for all tests #6639

Merged
merged 3 commits into from
Nov 26, 2024
Merged

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Nov 25, 2024

Not worth to have strict type check by mypy for tests/* .

Not worth to have strict type check by mypy for tests/* .
Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

I am kind of ambivalent about this change, leaving up to @agoscinski.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 25, 2024

It was from a thing I learned from Rust. It usually not recommend to use unwrap in the source code but the unwrap is extremely suitable and the right thing for tests. I think it kinds of similar for mypy to python. If the tests fail it fail, and the tests go through the "happy path" of the codes. Type checking will sometime too strict to consider beyond the "happy path". I hope it is not too aggressive.

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.90%. Comparing base (ef60b66) to head (2c00740).
Report is 142 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6639      +/-   ##
==========================================
+ Coverage   77.51%   77.90%   +0.40%     
==========================================
  Files         560      567       +7     
  Lines       41444    42147     +703     
==========================================
+ Hits        32120    32831     +711     
+ Misses       9324     9316       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

I think keeping a static type checker still would improve readability, but it also increases the development time. Especially, my experience with mypy is that you can sink in a lot of time until you fixed something that is not even very relevant. Since the tests follow a simpler logic than the src code I would say we can loosen up on readability a bit for the tests a bit to speed up development time.

@unkcpz unkcpz merged commit 846c11f into aiidateam:main Nov 26, 2024
7 of 8 checks passed
@unkcpz unkcpz deleted the no-mypy-for-tests branch November 26, 2024 09:30
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.

3 participants