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

Pre-commit hook with black #752

Open
shashank88 opened this issue Apr 27, 2019 · 13 comments
Open

Pre-commit hook with black #752

shashank88 opened this issue Apr 27, 2019 · 13 comments
Assignees

Comments

@shashank88
Copy link
Contributor

I have been using a pre-commit hook that does auto-formatting via black in my personal projects and was thinking if it would be a good idea to do something similar here?
@bmoscon @yschimke Wdyt?
https://www.mattlayman.com/blog/2018/python-code-black/

It would require one large commit, but it should save more time going forward

@shashank88 shashank88 self-assigned this Apr 27, 2019
@shashank88
Copy link
Contributor Author

I set it up on my laptop, tested out the pre-commit hook - works pretty well. Didn't seem to be breaking any tests as far as I can see. The only issue being it will create a massive PR and probably cause merge conflicts in other PRs once merged, so I will wait for #747 to finish before merging this

@bmoscon
Copy link
Collaborator

bmoscon commented Apr 29, 2019

I've heard of it, but like the article mentions, code formatting is a touchy subject :) I'd like to see a PR with the changes to see more of what they'd entail

@yschimke
Copy link
Contributor

I'm for anything automated, e.g. for my personal projects (kotlin) I'm not using spotless in gradle. Ultimately I've worked in enough codebases that I've learned to accept and follow the defaults that are already there. But I don't want to argue, just want to run a command to cleanup as needed.

@bmoscon
Copy link
Collaborator

bmoscon commented Apr 29, 2019

I agree with @yschimke. The only thing I'd argue about are 80 character line limits, those are just outdated and pointless :)

@shashank88
Copy link
Contributor Author

The change is on my laptop right now, will push it to a branch at night so you guys can see how it looks. @bmoscon Black is interesting in this regard as they chose 88 as the line limit after doing some research! But it's configurable so I am not fussed about 110 as long as it's consistent.

@shashank88
Copy link
Contributor Author

shashank88@e8d19ed is with the default autoformatting (88 line limit)

@pablojim
Copy link

I'm very pro this change

@bmoscon
Copy link
Collaborator

bmoscon commented Apr 29, 2019

I certainly won't do anything to stop the change, but I'm definitely anti this change. for the following reasons:

  • too many of the changes convert reasonable single lines of code to 2-3 lines of new code making them harder to read in my opinion
  • by changing this many lines you're effectively removing the usefulness of git blame

@shashank88
Copy link
Contributor Author

  • So the first thing can be easily addressed by setting the black limit-limit to 110. The reasoning they give for 88 is here: https://github.com/ambv/black#line-length .I can change the limit and resend the PR if you want.
  • I already did the Pycodestyle fixes once so I think I have already spoiled the git-blame to some extent :P, The idea of using these tools is to not worry about these things in code-reviews and reduce the effort going forward, but the one-time git-blame spoiling is something that can't be worked around.

@shashank88
Copy link
Contributor Author

shashank88 commented May 8, 2019

@bmoscon Do you think it's worth me resending a PR with 110 line limit? Happy to close this if you are not pro this change

@yschimke
Copy link
Contributor

yschimke commented May 8, 2019

@shashank88 resend with the 110 line limit, and let's reevaluate. OSS projects have to make similar calls all the time, hopefully it's tolerable after the line limit change.

@shashank88
Copy link
Contributor Author

@yschimke @bmoscon here is a PR with 110 line length: #769
Need to clean up commits etc. but this is just to get a rough diea

@DominikMChrist
Copy link

I would argue for a 120 (or 121) character limit, based on the line length in github.
See e.g. https://stackoverflow.com/questions/22207920/what-is-githubs-character-limit-or-line-length-for-viewing-files-on-github

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

5 participants