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

[REQUEST] Make use of pre-commit for running linters #355

Closed
ssbarnea opened this issue Oct 7, 2020 · 7 comments
Closed

[REQUEST] Make use of pre-commit for running linters #355

ssbarnea opened this issue Oct 7, 2020 · 7 comments
Assignees

Comments

@ssbarnea
Copy link
Contributor

ssbarnea commented Oct 7, 2020

I would like to suggest using pre-commit tool (not hook!) for orchestrating linters during development. Currently rich has two linters: black and mypy*.

How would you improve Rich?

It would be much easier to run linting, much easier to enable additional checks (like newline consistency) and make is extremely easy to bump linters.

Projects like black, tox, pytest are already using it for a good number of years.

What problem does it solved for you?

It would make easier to test code locally before making a PR.

If you do not have anything against it, I can easily make a PR to demonstrate it (probably less than 5min to do it).

@willmcgugan
Copy link
Collaborator

I'm willing to give it a try. I think @hedythedev found some issues with pre-commit, but as long as its optional I don't see a problem.

@ssbarnea
Copy link
Contributor Author

ssbarnea commented Oct 8, 2020

I am quite curious which issues @hedythedev identified. I even made a pre-commit presentation back in March around how it works.

ssbarnea added a commit to ssbarnea/rich that referenced this issue Oct 8, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.
- `tox -e lint` and `make lint` effectively do the same thing,
  the only difference is that tox one installs pre-commit. If desired
  we could call tox directly from Makefile.

Fixes: Textualize#355
@hedyhli
Copy link
Contributor

hedyhli commented Oct 8, 2020

I had trouble with installing the hook itself, but will try again when I have the time. It uses --user installs which wasn't possible in my environment. However I think it's a good idea overall

@ssbarnea
Copy link
Contributor Author

ssbarnea commented Oct 8, 2020

While I used pre-commit myself for probable more than 3 years, I almost never use its hook feature. I prefer to be able to bypass it and I got a habbit of running it manually.

I even have alias pc='pre-commit run -a' in my shell. The real value is not the hook, but how easy it makes to manage the lifecycle of the linters (add new, upgrade, run, avoid taking too much disk space, isolate/contain). If curious about a more heavy use of it, take a look at https://github.com/ansible-community/molecule/blob/master/.pre-commit-config.yaml

@Edward-Knight
Copy link
Contributor

It might also be useful to add some of the built-in pre-commit checks as well (like end-of-file-fixer or trailing-whitespace)

@ssbarnea
Copy link
Contributor Author

I did not want to pollute the original PR with new checks but my plan was to do it in follow-ups, so we can keep them small and easy to review.

ssbarnea added a commit to ssbarnea/rich that referenced this issue Oct 10, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.

Fixes: Textualize#355
ssbarnea added a commit to ssbarnea/rich that referenced this issue Oct 10, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.

Fixes: Textualize#355
ssbarnea added a commit to ssbarnea/rich that referenced this issue Oct 10, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.

Fixes: Textualize#355
ssbarnea added a commit to ssbarnea/rich that referenced this issue Oct 11, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.

Fixes: Textualize#355
ssbarnea added a commit to ssbarnea/rich that referenced this issue Oct 24, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.

Fixes: Textualize#355
ssbarnea added a commit to ssbarnea/rich that referenced this issue Nov 12, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.

Fixes: Textualize#355
ssbarnea added a commit to ssbarnea/rich that referenced this issue Nov 12, 2020
Adopts pre-commit for running already existing linters.

- Adds "make lint" as generic command
- Linters are removed from requirements-dev.txt as they are now
  managed by pre-commit itself. This avoids potential conflicts
  between linters as pre-commit installs each linter on its own
  environment.

Fixes: Textualize#355
@henryiii
Copy link
Contributor

henryiii commented Jan 8, 2021

This might be helpful; in Scikit-HEP we are using pre-commit everywhere. https://scikit-hep.org/developer/style. Also you can use https://pre-commit.ci if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants