arctic
arctic copied to clipboard
man-group
Pre-commit hook with black
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
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 https://github.com/manahl/arctic/pull/747 to finish before merging this
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
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.
I agree with @yschimke. The only thing I'd argue about are 80 character line limits, those are just outdated and pointless :)
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.
https://github.com/shashank88/arctic/commit/e8d19edcb8699a1c62776fd5a471feba19bb373b is with the default autoformatting (88 line limit)
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
- 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.
@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
@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.
@yschimke @bmoscon here is a PR with 110 line length: https://github.com/manahl/arctic/pull/769 Need to clean up commits etc. but this is just to get a rough diea
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
