Replace linter and formatter with ruff and mypy#392
Conversation
LecrisUT
left a comment
LecrisUT
left a comment
There was a problem hiding this comment.
Also, just make sure when you apply pre-commit it's a separate commit, and we don't squash commit for this PR
python/spglib/spglib.py
Outdated
|
|
||
| try: | ||
| from spglib import _spglib as spg | ||
| from spglib import _spglib as spg # type: ignore |
There was a problem hiding this comment.
Why the ignore? Also should scope ignores
There was a problem hiding this comment.
Without ignoring, mypy says
error: Module "spglib" has no attribute "_spglib" [attr-defined]
There was a problem hiding this comment.
Yeah, but that's a genuine issue:
- We should be using relative paths there
- We should be providing a
_spglib.pyito type-hint the C interface, even if only internally
There was a problem hiding this comment.
The relative path did not persuade mypy
from .spglib import _spglib as spg
# -> Module "spglib.spglib" has no attribute "_spglib" [attr-defined]
|
Also worth referencing the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #392 +/- ##
========================================
Coverage 83.86% 83.86%
========================================
Files 24 24
Lines 8180 8180
========================================
Hits 6860 6860
Misses 1320 1320
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Please split the "add mypy/ruff configuration" and "apply mypy/ruff” as separate commits in 31b2842. You can run |
lan496
changed the title
LecrisUT
left a comment
LecrisUT
left a comment
There was a problem hiding this comment.
👍 for the current setup as an initial implementation. Mind if I rebase your commits a bit before merging?
.pre-commit-config.yaml
Outdated
| hooks: | ||
| - id: mypy | ||
| exclude: database | ||
| pass_filenames: false |
There was a problem hiding this comment.
The size of the python bindings is very small, so might as well run mypy on all sources defined in pyproject.toml, i.e. running
$ mypy --ignore-missing-imports --scripts-are-modules
|
@LecrisUT No problem. I'll leave the rest to you. |
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
|
Sorry, had to change the committer because I've split the commits. Btw, can you add Also make sure to merge this with a merge commit or rebase to keep the history |
|
Thanks to tidy commits. I've updated the branch protection rule. Is it OK? |
|
Yep, that looks good. I'll bother you again later when I enable Fedora38 |
3 participants
Closes #364