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 pre-commit checks #597

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Conversation

erikbosch
Copy link
Contributor

@erikbosch erikbosch commented Jul 10, 2023

This will introduce pre-commit checks for kuksa-val. We already have it for kuksa.val.feeders

What it will do:

  • For most files check that there is a line-break at end-of-file
  • For most files verify that you do not have trailing blanks
  • For python verify that flake8 pass, checking for instance line lengths

Suggest approach is to have a pre-commit hook installed on your local machine, then the check will be performed before you commit and this check shall be guaranteed to pass. But you may work reactively as well and fix issues if build fails and upload a new version. It only checks files affected by the commit/PR, but it checks the whole file, so initially you may need to fix some old sins.

Caveat: There might be cases where we do not want files to be changed, I took some settings "inherited" from kuksa.val.feeders. If you would notice that the checker wants to change files that should not be changed, then we may need to tweak the setting.

Make sure that we do not have trailing blanks and so on
and that Python code is reasonably formatted
@erikbosch erikbosch marked this pull request as ready for review July 12, 2023 06:23
Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment

- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.2.0
hooks:
- id: trailing-whitespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add another hook for python linter? Then you do not need to wait until CI runs through and clippy for rust files maybe. Just a thought

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flake8 and pylint has similar checks, but with some differences. At the moment we do not do any linter checks with pylint/mypy in CI, but in the long run we better should. In feeder we use pylint/mypy, but only fail on errors. I did by the way try mypy some time ago on kuksa-client but it gave some false positives as it could not really understand the gRPC integration correctly, so there are some tweaks needed if we are to use it.

It is possiblw to run pylint as part of pre-commit, see https://pylint.pycqa.org/en/latest/user_guide/installation/pre-commit-integration.html, but it requires that you do a local install. That is no problem for us working with Python but I am not sure if our rust/c++-guys want to do that, as it might be needed even if you have not changed any Python files.

I suggest we take extending/improving checks as a later step.

@erikbosch erikbosch merged commit d5fbb7c into eclipse:master Jul 20, 2023
@erikbosch erikbosch deleted the erik_precommit branch September 29, 2023 09:29
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 this pull request may close these issues.

2 participants