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

Reinclude linters as pre-commit hooks #48

Open
fredjaya opened this issue Nov 1, 2023 · 3 comments
Open

Reinclude linters as pre-commit hooks #48

fredjaya opened this issue Nov 1, 2023 · 3 comments
Assignees
Labels
docs: install Related to documentation for installing the development environment

Comments

@fredjaya
Copy link
Collaborator

fredjaya commented Nov 1, 2023

Already run as a GitHub Action, but mainly so you can run locally. Add to dev_install.

#23 (comment)

@fredjaya fredjaya self-assigned this Nov 1, 2023
@fredjaya fredjaya added the docs: install Related to documentation for installing the development environment label Nov 1, 2023
@GavinHuttley
Copy link
Collaborator

I've played with this and it's now clear my original objective cannot be solved easily, so I will discuss the purpose of this here and then suggest the next step.

We want all code in a PR to have been run through the correct versions of black and isort. This means when we review, we focus our comments on the code logic, not the contributor's adherence to our style requirements.

GitHub actions run by the cogent3 GitHub organisation cannot modify the contributor's repo. So we need the contributor to do it either on their machine or in their own GitHub account before making the PR.

So I think we can do two things here:

  1. add a new GitHub action which is run (just on ubuntu, one version of python) before the test suite and fails if the black/isort commands fail
  2. adapt the existing GitHub linter action so that it is triggered on the user's GitHub account on a push. Tell them how to set it up in our dev instructions. By using the versions of black / isort and OS as per our check, this should be the least burden on users.)

@khiron what do you think?

@khiron
Copy link
Collaborator

khiron commented Nov 5, 2023

I definitely like the idea of running black/isort on just one version of python for a quick check for all pushes to the dev branch of the cogent3 repository. We can also just check-out changed code in that workflow, to make it extra quick. Also, linting before testing is a good call. Fail early is good.

pre-commit hooks in the local git repository should be the preferred strategy for devs who don't like getting their PR's bounced.

I recall developing a strategy for misnaming pre-commit hooks in the repository that the developer just had to rename locally to implement them (and a .gitignore strategy that didn't consider the renaming to be a committable change). That way, the code was part of the repo, but it only got executed locally if the developer made the required rename.

EDIT: Yup found it in the scratch repo I built for setting up scriv.

We add the following to .gitignore
.github/local-hooks/pre-commit

and add the following 3 files to the /.github/ folder

https://github.com/khiron/testscriv/tree/master/.github/local-hooks

[just made this scratch repo public]

@GavinHuttley
Copy link
Collaborator

I agree the dev should introduce the precommit on their machine, but again thinking of the shortest path for slightly less sophisticated dev's, making it so their GitHub repo does it at least means they don't need to do more command line until they're ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs: install Related to documentation for installing the development environment
Projects
None yet
Development

No branches or pull requests

3 participants