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

Add lint to pre-commit and/or to CI #364

Open
jonperron opened this issue Dec 21, 2024 · 1 comment · May be fixed by #387
Open

Add lint to pre-commit and/or to CI #364

jonperron opened this issue Dec 21, 2024 · 1 comment · May be fixed by #387

Comments

@jonperron
Copy link
Contributor

Problem

I noticed that neither the CI pipeline nor the commit process includes a linter to enforce best practices.

Solution

I would propose to add at least ruff as pre-commit, so code quality is checked before pushing to repo. You can check an example of implementation here.

Alternatives

Can also work with pylint, flake8 or black.

@jonperron
Copy link
Contributor Author

@da-ekchajzer let me know if this is interesting for the project !

As a pre-commit, we can use it following the "Boy scout rule" to improve the code quality of the project.
As a linter, it will be more complex to use it, I will need a bit more of work as the linter find a lot of "errors"

Found 92 errors.
No fixes available (7 hidden fixes can be enabled with the `--unsafe-fixes` option).

These errors are mainly import issues F405 (may be undefined, or defined from star imports), F401 (imported but unused) or E721 (Use is and is not for type comparisons, or isinstance() for isinstance checks).

Thus, ff we go for a linter, we should define what we accept as issues and what we refuses, code like

boaviztapi/dto/component/__init__.py:5:18: F401 `.ram.RAM` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
  |
3 | from .disk import Disk
4 | from .other import Case, Motherboard, PowerSupply
5 | from .ram import RAM
  |                  ^^^ F401
  |
  = help: Use an explicit re-export: `RAM as RAM`

can be safely accepted in my opinion.

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 a pull request may close this issue.

1 participant