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

ci: Add pre-commit configs #215

Merged
merged 1 commit into from Feb 2, 2021
Merged

ci: Add pre-commit configs #215

merged 1 commit into from Feb 2, 2021

Conversation

jidicula
Copy link
Contributor

Checklist

  • I've given this PR a concise, self-descriptive, and meaningful title
  • I've linked relevant issues in the PR body
  • I've applied the relevant labels to this PR
  • I've added relevant tests for my contribution
  • I've updated the documentation and/or added correct docstrings
  • I've assigned a reviewer

Description

Why this change was necessary
We want to enforce file size limits on commits to ensure contributors
don't accidentally commit massive files to the repo and pollute the
history. This should be checked both locally and on GitHub.

What this change does

  • Adds pre-commit as an optional dev dependency
  • Adds pre-commit hooks for YAML check, merge conflict string
    check, AST check, and large file (>500kb) check
  • Adds GitHub Action to run above pre-commit checks on each PR and
    push to master
  • Updates documentation to instruct contributors to install dev dependencies

Additional context/notes/links

Linked issues

Resolves #199

@jidicula jidicula requested a review from a team January 30, 2021 17:12
@jidicula jidicula added card:TO_REVIEW Assigned PR under review installation Alters the installation of the shimming-toolbox labels Jan 30, 2021
Resolves #199

**Why this change was necessary**
We want to enforce file size limits on commits to ensure contributors
don't accidentally commit massive files to the repo and pollute the
history. This should be checked both locally and on GitHub.

**What this change does**
 - Adds `pre-commit` as an optional `dev` dependency
 - Adds `pre-commit` hooks for YAML check, merge conflict string
 check, AST check, and large file (>500kb) check
 - Adds GitHub Action to run above pre-commit checks on each PR and
 push to master
 - Updates documentation to instruct contributors to install `dev` dependencies

**Additional context/notes/links**
 - https://github.com/pre-commit/pre-commit-hooks#check-added-large-files
 - https://pre-commit.com
@po09i
Copy link
Member

po09i commented Feb 2, 2021

Are the local hooks always run when we locally commit something or we need to manually run pre-commit to check?

@jidicula
Copy link
Contributor Author

jidicula commented Feb 2, 2021

Once the project has been reinstalled with pip install -e ".[docs,dev]" and you've run pre-commit install once, the hooks will run automatically when you locally commit something.

Copy link
Member

@po09i po09i left a comment

Choose a reason for hiding this comment

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

Awesome, I'm scared to test it but if you have then it LGTM!

@jidicula jidicula merged commit d4de1a1 into master Feb 2, 2021
@jidicula
Copy link
Contributor Author

jidicula commented Feb 2, 2021

@shimming-toolbox/editors once this PR is closed, please run pip install -e ".[docs,dev]", then run pre-commit install.

@jidicula jidicula deleted the ji/issue-199-add-pre-commit branch February 2, 2021 18:17
@joshuacwnewton
Copy link
Member

joshuacwnewton commented Feb 8, 2021

kousu pushed a commit that referenced this pull request Mar 20, 2021
Resolves #199

**Why this change was necessary**
We want to enforce file size limits on commits to ensure contributors
don't accidentally commit massive files to the repo and pollute the
history. This should be checked both locally and on GitHub.

**What this change does**
 - Adds `pre-commit` as an optional `dev` dependency
 - Adds `pre-commit` hooks for YAML check, merge conflict string
 check, AST check, and large file (>500kb) check
 - Adds GitHub Action to run above pre-commit checks on each PR and
 push to master
 - Updates documentation to instruct contributors to install `dev` dependencies

**Additional context/notes/links**
 - https://github.com/pre-commit/pre-commit-hooks#check-added-large-files
 - https://pre-commit.com
@vasudev-sharma
Copy link

Inspired by this PR, we're implementing the same feature in another sister project (AxonDeepSeg) see here.

The pre-commit hook in this PR requires the developer/contributor to do the mandatory step pre-commit install so that the checks are being caught locally.

There might be instances where the developer might forget this step and this would lead to the checks being skipped locally.

In order to avoid this, for this, in the PR axondeepseg/axondeepseg#464, we are thinking to do add pre-commit install in the build script (setup.py) as a post-install step via setuptools.

I wish to know did you consider this option for installing pre-commit hooks?

  1. If yes, may I know why you didn't plan on keeping it?

  2. If no, then may I know your thoughts on this?

@jidicula
Copy link
Contributor Author

I wish to know did you consider this option for installing pre-commit hooks?

  1. If yes, may I know why you didn't plan on keeping it?
  2. If no, then may I know your thoughts on this?

I actually didn't know this was an option! It looks like it's viable, with the only caveat that it creates a bit of dependence on setup.py magic. Just remember to attempt triggering it only if the dev extra is the build option. My other worry about setup.py dependence is that it makes shifting to the recommended pyproject.toml dependency specification a bit harder - hopefully PSF comes up with something that replicates that functionality.

@vasudev-sharma
Copy link

Thanks @jidicula for your valuable inputs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
card:TO_REVIEW Assigned PR under review installation Alters the installation of the shimming-toolbox
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit file size on commit
4 participants