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

WIP | Add pre-commit #673

Closed
wants to merge 3 commits into from
Closed

WIP | Add pre-commit #673

wants to merge 3 commits into from

Conversation

Pacu2
Copy link
Contributor

@Pacu2 Pacu2 commented Apr 27, 2020

Adding simple pre-commit as discussed in #660
Unfortunately, darker does not support pre-commit yet but opened a related pull request to change it in akaihola/darker#2

To enable pre-commit one needs simply:

  1. python -m pip install -r requirements/dev.txt
  2. pre-commit install

this PR consists of 3 commits:

  1. requirements update
  2. pre-commit configuration
  3. fix for all trailing whitespace/incorrect end of file lines generated by the pre-commit

@SanketDG
Copy link
Contributor

This looks like a great start!

Would you consider using darker given it's obscure enough?

I am still on the black bandwagon and I can still believe we can do it right with what I said in #660 (comment) albeit with a few blockers here and there.

Also it looks like pip-tools is being used and not used (there is an external requirements.txt file)

@timgl
Copy link
Collaborator

timgl commented Apr 28, 2020

Thanks for doing this.

This does add another way of doing precommits, we already use husky for JS and I don't think they play nicely together. I personally also prefer how Husky just adds the formatted code to your commit, rather than failing and having to manually add the code back. The other nice thing is it 'just works', without manually having to install something.

@SanketDG
Copy link
Contributor

@timgl Yeah, pre-commit does the same. it matters more what tool you are using (prettier/black that autoformats vs something else that just reports and exits)

@Pacu2
Copy link
Contributor Author

Pacu2 commented Apr 30, 2020

@timgl Husky requires a package manager, and NodeJS to run hooks. This way, backend engineers would need to set up (and maintain) the frontend ecosystem.

Pre-commit, on the other side, just works. It supports a whole variety of scripts, no matter the programming language they use.

Not sure how to use backend-oriented hooks with Husky, however, pre-commit supports several frontend-oriented ones, not to mention eslint, tslint, prettier...

@timgl
Copy link
Collaborator

timgl commented May 4, 2020

Happy either way! I do think we should make sure this PR doesn't break frontend formatting.

@timgl
Copy link
Collaborator

timgl commented Jun 11, 2020

@Pacu2 Hello again, sorry this has stalled for a while. Do you still have interest in picking this up?

@Pacu2
Copy link
Contributor Author

Pacu2 commented Jun 11, 2020

@timgl sure will do in 24h

@timgl
Copy link
Collaborator

timgl commented Jun 19, 2020

This was done in #1043, but really appreciate the work!

@timgl timgl closed this Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants