-
Notifications
You must be signed in to change notification settings - Fork 368
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
Add pre-commit-config and improve linting configuration #1724
Conversation
@@ -482,7 +482,7 @@ def _set_attr(self, atttype, fargs, attname: str, val): | |||
self._set_attr(sub_atttype, subargs, attname2, v2) | |||
if "Encoding" in subargs: | |||
del subargs["Encoding"] | |||
fargs[attname] = atttype(**subargs) | |||
fargs[attname] = atttype(**subargs) # type: ignore[operator] |
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.
Otherwise mypy fails here with,
asyncua/common/xmlimporter.py:485: error: "DataclassInstance" not callable [operator]
not really sure why
let's try |
rev: 'v0.6.9' | ||
hooks: | ||
- id: ruff | ||
args: ['--fix'] |
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.
oops I merged it but that line looks very strange... why fix? there is not reason to run fix for linting, isnt'it?
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.
No that line is correct. That way you can use the same config locally and in CI. Locally you run with a fix and it fixes. And it CI it will still exit with the exit status > 0, so the linting CI would fail and you would see what it would have fixed.
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.
If there are any difficulties with this don't hesitate to mention me.
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.
then you hav ruff check --diff, which is meant exactly for this, isnt'it?
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.
Well it's how they say it could be used with pre-commit https://github.com/astral-sh/ruff-pre-commit?tab=readme-ov-file#using-ruff-with-pre-commit
Without it, it means you have to manually fix those linting errors (or manually run ruff check --fix
). Up to you.
About ruff check --diff
not sure, I just copy past whatever the ruff team says the .pre-commit-config.yaml
for ruff should be :)
I made a PR recently and it was a bit frustrating due to lint errors. So I had to fix both mypy and ruff each time just to get some feed back from tests in CI.
This improves a bit the linting using experience with pre-commit.
You can do,
and it would run linters (mypy, ruff) locally when creating a new commit (on changed files). It's also possible to run linters for all files with
pre-commit run -a
.This ensures that all the lint dependencies use the same version locally and in CI. Also it would display all lint errors in CI at once (rather than displaying ruff errors, having to fix them, only to see the mypy errors as currently)
I also moved linting to a separate job in CI, so it doesn't block tests if it fails. As there is no need to run them for different python versions.