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

feat: introduce pre-commit and configure most existing linters in it #118

Conversation

tk-woven
Copy link
Collaborator

@tk-woven tk-woven commented Aug 12, 2022

Explanation

This is toward #114 . This introduces the pre-commit tool which is configured to run all hooks that we previously ran locally via standard shell scripts.

This MR also runs the hooks on all files resulting in some being touched up.

SuperLinter (used in CI) still provides some checks that we do not cover here yet. This also updates the Dockerfile to symlink the pip installation to /usr/bin to support some tools that expect to find pip installed there.

Reading Guide

Most changes in this MR are source reflows from the linters. The exceptions are

  1. deleted .githooks
  2. introduced .pre-commit-config.yaml
  3. modified Dockerfile and Makefile
  4. updated some Docker/venv/getting-started docs
  5. run pre-commit as a CI step in .github/workflows/pre-merge.yml

Limitations

Setting up the hooks in the Docker environment is a bit awkward because the user first has to install git and then trust the DGP repo. We could do this as part of the Dockerfile, but that would mean installing git. I did not move to install git in this work.

The usual concern of people running a virtual environment with a Python version different than what we use in CI persists.


This change is Reviewable

This also runs the hooks on all files resulting in some being touched
up.

This replaces all previously-local githooks with hooks managed by
pre-commit. SuperLinter (used in CI) still provides some checks that we
do not cover here yet. This also updates the Dockerfile to symlink the
pip installation to /usr/bin to support some tools that expect to find
pip installed there.
@tk-woven tk-woven force-pushed the feat/tyler-kowallis/use-pre-commit-for-linting branch from f70e41c to a6616cc Compare August 12, 2022 05:16
@tk-woven tk-woven marked this pull request as ready for review August 12, 2022 05:21
Copy link
Collaborator

@wadimkehl wadimkehl left a comment

Choose a reason for hiding this comment

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

Nice! Looking forward to using that ;) :lgtm:

Reviewed 49 of 49 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @chrisochoatri and @kuanleetri)

@tk-woven
Copy link
Collaborator Author

Thanks, will merge now so everyone can use it! Tail-end work will have me replace the last linters from SuperLinter with hooks in pre-commit, and then we'll have single-entrypoint linting!

@tk-woven tk-woven merged commit 9752a54 into TRI-ML:master Aug 12, 2022
@tk-woven tk-woven deleted the feat/tyler-kowallis/use-pre-commit-for-linting branch September 6, 2022 00:21
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