-
Notifications
You must be signed in to change notification settings - Fork 82
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
based linting and formatting
#35
Conversation
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
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.
Is it possible to install pre-commit locally, like vLLM? We could document it if it is possible.
Signed-off-by: Harry Mellor <[email protected]>
Yes, of course. I added |
@ApostaC since the repo has been unformatted until now, there are lots of changes that the hooks want to make. Would you prefer if I do this in this PR, or in a follow up? (meaning that this PR would merge with pre-commit failing) |
Signed-off-by: Harry Mellor <[email protected]>
I would prefer fixing the pre-commit in this PR to make sure the CI is always green, @ApostaC what's your thought? |
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
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.
Thanks for the effort! Looks good to me!
In the meantime, can you add something in the README.md about how to run pre-commit check locally (something like pre-commit run --all-files
maybe?)
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
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.
LGTM!
* Add pre-commit workflow * Add actionlint * Add generic hooks * Add black, isort, shellcheck * Add requirements and markdown linting * Add toml * Add Dockerfile * Add codespell * Use Node.js version of `markdownlint` * Add `requirements-lint.txt` * Use CLI version of Node.js `markdownlint` * Add `pre-commit` instructions to `Contributing` * `pre-commit run -a` automatic fixes * Exclude helm templates from `check-yaml` * Comment hooks that require installed tools * Make `codespell` happy * Make `actionlint` happy * Disable `shellcheck` until it can be installed properly * Make `markdownlint` happy * Add note about running pre-commit --------- Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: 0xThresh.eth <[email protected]>
The hooks added are as follows:
actionlint
- GitHub Actionscheck-{json/toml/yaml}
- files with these extensionsend-of-file-fixer
andtrailing-whitespace
- all filesrequirements-txt-fixer
- Python requirements fileshadolint
- Dockerfiles (currently disabled)helmlint
- Helm charts (currently disabled)black
andisort
- Pythonshellcheck
- shell (currently disabled)markdownlint
- markdowncodespell
- spelling