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 black, pre-commit, or ruff action to aid in code style for developers #1356

Closed
zm711 opened this issue Dec 1, 2023 · 5 comments
Closed

Comments

@zm711
Copy link
Contributor

zm711 commented Dec 1, 2023

Is your feature request related to a problem? Please describe.
Pep8 speaks puts the onus on the developer to fix all the mistakes which are annoying to deal with. But using black/ruff as the developer would lead to huge diffs unless it is supported overall by the maintainers and eventually applied to the whole code base.

Based on discussion with @alejoe91 in #1318 (and I'm sure very much against the wishes of @samuelgarcia ), it might be nice to at least think about adding an action that would get rid of the need to have pep8 speaks on and that would occur behind the scenes for the developer. In addition it will eventually make code review easier once it has been applied to the whole code base.

In addition some pep8 speaks error codes are a little hard to parse (at least for me) and fine, which just leads to a bunch of pep8 or pep8 compliance commits which could all be solved with one GH action making the pep8 fixes automatically.

Describe the solution you'd like
I think the easiest would be to add a pre-commit so that the code is autoformatted to the desired pep8 specifications (even if its not black-ified). I know ruff gives some nice flexibility in picking what to enforce (and in general is also pretty compatible with black-ified code).

Describe alternatives you've considered
Pep8 speaks could be shut off and it could go back to the wild west of submitting code as is.

Additional context
NA

@alejoe91
Copy link
Contributor

alejoe91 commented Dec 1, 2023

I vote for pre-commit CI with black and line length of 120 (which IMO makes code better readable). We have been doing it for half a year now in Spikeinterface and it's working very smoothly. Thanks for bringing this up @zm711

@samuelgarcia
Copy link
Contributor

For shuting off pep8, I am not going to be hard to convince.
For pre-commit and black I prefer to to not tell my deep feeling.

@apdavison
Copy link
Member

also see discussion in #922

I am comfortable with using black, as long as the maximum line length is set to a sensible value (120 would be fine).

I'm not so sure about pre-commit hooks, which I think raise the difficulty for new contributors. GitHub doesn't allow pre-receive hooks, but what about having a GH Action to format received code?

@zm711
Copy link
Contributor Author

zm711 commented Dec 3, 2023

Currently the pep8 action which has turned back on yells at developers for 99, which is not sensible (in my opinion) at all. But although I use black myself, I'm neutral on the actual linter (I'm just pro using some sort of automatic linter). I know @alejoe91 and @h-mayorquin were able to get a super nice pre-commit working on spikeinterface that actually makes it super easy (ie the new contributor actually does nothing--the CI does it all). But that level of GH action is beyond my skills so maybe one of them can weigh in on the "how to do".

@zm711
Copy link
Contributor Author

zm711 commented Jan 26, 2024

Closed by #1376.

@zm711 zm711 closed this as completed Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants