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

Add pre-commit config to enable auto-formatter and linting on commit #2291

Open
djhoese opened this issue Feb 4, 2022 · 5 comments · May be fixed by #2503
Open

Add pre-commit config to enable auto-formatter and linting on commit #2291

djhoese opened this issue Feb 4, 2022 · 5 comments · May be fixed by #2503

Comments

@djhoese
Copy link
Member

djhoese commented Feb 4, 2022

See https://pre-commit.com/

Related #2146

A pre-commit config is a great utility/tool to take advantage of to avoid code style, security, and other types of issues before the code is ever committed. With the additional use of pre-commit.ci, we can enable the execution of it on PRs.

We could borrow from projects like:

I think a good starting point would be black and flake8 and see how bad the complaints are. We may not be able to enable pre-commit.ci immediately as it has to succeed on the entire repository and VisPy docstrings are just not consistent enough for that.

@aganders3
Copy link
Contributor

I just noticed that linting with python make test flake seems to be ignoring failures. See any recent CI run and expand the "Lint with flake8" section.

This is due to a change in flake8 where its main function used to exit on failure, but now only returns a status code. So this always succeeds:
https://github.com/vispy/vispy/blob/e71b275f0bd92d3fc082fdf87f11c490705ef76a/vispy/testing/_runners.py#LL167C19-L167C19

I am happy to make a PR to fix this small issue (and the linting issues that have crept in as a result), but when searching for an existing issue I found this one. Instead I would be interested in adding a pre-commit config if that's the direction you'd lake to take this anyway.

@djhoese
Copy link
Member Author

djhoese commented Jun 21, 2023

I'm good with a pre-commit. I'd also be OK trying to use ruff instead of flake8 and all the other tools. I can also enable pre-commit.ci on this repository so it automatically runs pre-commit.

Not sure how the other @vispy/core maintainers feel, but I kind of just want to enable black on the whole repository.

@larsoner
Copy link
Member

but I kind of just want to enable black on the whole repository.

numpydoc did this about a year ago and we did it a few months ago for MNE-Python. No regrets. Same goes for ruff replacing flake8 and pyflakes.

@aganders3
Copy link
Contributor

Again I'm happy to do some of the tedious work if others are willing to provide sanity checks and review. I can open a PR at least to see what this could look like if that's welcome.

A quick test locally shows some issues where there is a bit of Python magic going on (undefined symbols, etc.) so my instinct is to exclude some of the files in codegen and vispy/gloo/gl, at least to start.

@djhoese
Copy link
Member Author

djhoese commented Jun 22, 2023

I think excluding codegen is fine. I thought I cleaned up most of the flake8 issues in it, but definitely any of the files that codegen generates (which I think are the ones in vispy/gloo/gl) should be excluded.

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

Successfully merging a pull request may close this issue.

3 participants