-
Notifications
You must be signed in to change notification settings - Fork 167
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 and fix findings #601
Conversation
I am aching to do this with the codebase but it will create conflicts on opened PRs so I am inclined to merge as much as possible before we merge this one to minimize the amount of rebasing hell we need :D |
@jaimergp going to strip down the pre-commit config to reduce the amount of changes... more pre-commit hooks in be incrementally added via more PRs. |
Ok, that makes more sense. Maybe using |
@dbast I've just added the precommit.ci app to this repo. https://results.pre-commit.ci/repo/github/51571636
|
Co-authored-by: Jannis Leidel <jannis@leidel.info>
@jezdez Any idea how to extend the pre-commit,ci to be able to execute hooks that need additional tools? like the shellcheck hook which requires Docker (or in other cases hooks that require golang)? That is pretty easy with the pre-commit action, as there are anyways more tools pre-installed on the action runners and also extending those can be done via an action step running before the pre-commit step. This repo has many shellscripts as part of the code and as part of the examples and as shellcheck pretty good detects all kind of issues like unquoted variables, posix compatibility, missing error handling etc. This PR only checks the example shell scripts via shellcheck. The shell scripts in code are templated and require rendering before linting them via shell check; that is done in this PR #600 The pre-commit ci action is also also pretty fast as it also uses caching of the pre-commit hooks specific to this repo. One run = 23 secs (including shellcheck). |
This reverts commit f42f079.
@jezdez we can also invite the (public) renovate bot which to keeps the .pre-commit-config.yml hooks versions updated via PRs it proposes... renovate can also update arbitrary pinning versions, if the right regex pattern are added. That would e.g. help to pin the micromamba version in #605 to have stable CI and have it updated by renovate. |
What about this alternative? https://pypi.org/project/shellcheck-py/ |
@jaimergp hah. good finding. trying that. |
Description
Inspired by the .pre-commit-config.yaml in conda/conda-build/conda-pack where similar PRs have been done in the past.
The .pre-commit-config.yaml has been stripped down to mainly only contain the flake8 and shellcheck hooks for keeping the number of changed lines as small as possible. More hooks (isort, pyupgrade) can be incrementally enabled via more PRs. Similar PRs have been e.g. merged to conda without causing too much trouble with existing open PRs.
Getting some linting running automatically is of great help when reviewing PRs. This once requires all current finding being fixed, which is done as part of this PR.
This also finds some not existing code in conda_interface.py where the function
mkdir_p_sudo_safe(cache_dir)
does not exist.Checklist - did you ...
news
directory (using the template) for the next release's release notes?