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
Replace linter and formatter with ruff and mypy #392
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -40,7 +40,7 @@ | |||
import numpy as np | |||
|
|||
try: | |||
from spglib import _spglib as spg | |||
from spglib import _spglib as spg # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the ignore? Also should scope ignores
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without ignoring, mypy says
error: Module "spglib" has no attribute "_spglib" [attr-defined]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that's a genuine issue:
- We should be using relative paths there
- We should be providing a
_spglib.pyi
to type-hint the C interface, even if only internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for the current setup as an initial implementation. Mind if I rebase your commits a bit before merging?
.pre-commit-config.yaml
Outdated
exclude: database | ||
pass_filenames: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
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 |
Yep, that looks good. I'll bother you again later when I enable Fedora38 |
Closes #364