Fixed mypy sp check guidelines #4887
Conversation
|
Still 25 mypy errors are left, drafting this so that its easier to communicate changes |
There was a problem hiding this comment.
Thanks, I've triggered the workflows! I haven't reviewed all the files, because I understand that the PR is still a draft and you have yet to complete the changes.
Checking https://learn.scientific-python.org/development/guides/repo-review/?repo=pybamm-team/pybamm, a few more rules where we have problems have popped up. So we shouldn't mark this PR as one that closes the linked issue. Could you please replace the "Fixes" keyword in the PR description with "Related to"?
It is also possible to add sp-repo-review as a pre-commit hook so that new rules will be checked on every commit. I think we should investigate that and add it if running the checks is not too slow. I'm also curious to hear what @Saransh-cpp has to say about this.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4887 +/- ##
===========================================
- Coverage 98.71% 98.70% -0.01%
===========================================
Files 304 304
Lines 23495 23528 +33
===========================================
+ Hits 23192 23223 +31
- Misses 303 305 +2 ☔ View full report in Codecov by Sentry. |
|
It would be great to have repo-review as a pre-commit hook! @agriyakhetarpal maybe you can create a new independent issue for that? |
|
I've changed the PR description to related to and I also think it would be great to have a pre-commit hook for |
|
I've fixed the remaining errors too |
There was a problem hiding this comment.
Thanks for taking this up, @Rishab87! See my comment below about splitting this PR.
| @@ -252,6 +252,10 @@ concurrency = ["multiprocessing"] | |||
| ignore_missing_imports = true | |||
| allow_redefinition = true | |||
| disable_error_code = ["call-overload", "operator"] | |||
| strict = false | |||
There was a problem hiding this comment.
Given how big this PR is, it would actually be better to split it into multiple PRs, each one adding a new config option in pyproject.toml.
There was a problem hiding this comment.
ok so should I go ahead then and make a PR on one of the config first? or edit this one accordingly?
There was a problem hiding this comment.
You can keep this PR for one config, and add other configs in subsequent PRs.
There was a problem hiding this comment.
ok I'll create seperate PRs for different configs and then keep this one for the end, I think that would be faster for me
There was a problem hiding this comment.
I noticed many of the warn_unreachable error depends on errors from enable_error_code config, they're related to each other and their are total of 77 errors out of which 57 are from enable_error_code so I think creating a seperate PR would still be almost as big as this one, so should I still proceed with it?
There was a problem hiding this comment.
Yes, I think anything that reduces the diff and keeps this PR scoped to a specific change (or a few of them) would be great. Thanks for investigating!
There was a problem hiding this comment.
I've created the PR with just enable_error_code config: #4891, sorry for the delay lab tests going on
Co-authored-by: Saransh Chopra <[email protected]>
| @@ -48,7 +49,7 @@ | |||
| disc.process_model(model) | |||
|
|
|||
| # solve model | |||
| solutions = [None] * len(models) | |||
| solutions: Any = [None] * len(models) | |||
There was a problem hiding this comment.
Shouldn't this be something like list[Any]?
There was a problem hiding this comment.
yess, I'll change that
Description
Fixing mypy sp check guidelines
Related to #3489
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests