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

format SCM diffs only #320

Closed
gaborbernat opened this issue Jun 8, 2018 · 8 comments
Closed

format SCM diffs only #320

gaborbernat opened this issue Jun 8, 2018 · 8 comments
Labels
T: documentation Improvements to the docs (e.g. new topic, correction, etc)

Comments

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 8, 2018

Feature request: for projects not fully adopting black yet, would be nice if we could have some way of using black on the diffs only. I understand this is a non-trivial task. But should be possible to somehow get the diff against HEAD and keep only lines from that. Taking over the world one commit at a time?

@gaborbernat gaborbernat changed the title sort SCM diffs only format SCM diffs only Jun 8, 2018
@ambv
Copy link
Collaborator

ambv commented Jun 8, 2018

That goes against PEP 8 (consistency within a file) and the philosophy of Black. For adoption it's a better experience to create one sweeping commit that is easy to omit in git blame, rather than mix formatting changes with semantic changes for a long time.

We need a section of the docs that explains this.

@ambv ambv added the T: documentation Improvements to the docs (e.g. new topic, correction, etc) label Jun 8, 2018
@gaborbernat
Copy link
Contributor Author

I see. I was thinking more along lines for projects that have no true one style, that already violates pep 8.

@Lukas0907
Copy link

You can always reformat the whole file, and discard irrelevant changes with git checkout -p $FILE.

@crdoconnor
Copy link

crdoconnor commented Jun 9, 2018

For adoption it's a better experience to create one sweeping commit

I think that's a bad idea:

  • Rewriting code is inherently dangerous. Black's reformatting could cause a subtle bug (if it does, it is unlikely to be picked up on an enormous pull request) which might not be detected until it causes major problems. If its changes were isolated to pull requests that will actually be read then the chances of that happening are minimized.

  • It will still damage the usefulness of git history. You will be able to "git blame -i {sweeping commit}" provided you are using git blame on the command line, but if you use, say, annotate in pycharm or blame in github that option isn't available and those tools are rendered useless.

@carljm
Copy link
Collaborator

carljm commented Jun 9, 2018

Rewriting code is inherently dangerous. Black's reformatting could cause a subtle bug

Black's reformatting is AST-safe; the AST resulting from parsing the reformatted code is identical to the AST prior to the reformat. Black checks and verifies this identity. Therefore, the only way a Black reformat could cause a bug is if you have code doing something really unusual and unadvisable, like reading Python files from disk and manually parsing them with a regex, or similar.

@ambv
Copy link
Collaborator

ambv commented Jun 9, 2018

It will still damage the usefulness of git history.

blame != history. It's cleaner to have a single point in history reformatting everything, rather than a sprawling number of diffs that make semantic changes and move tokens around to improve style.

The easiest way to ignore a sweeping commit with git is git hyper-blame. If you don't want to or can't use that, then using git blame $BLACK_COMMIT^ -- $FILE is what you want (note the caret). You can easily replicate that in the GUI by clicking on a link to $BLACK_COMMIT (will be next to any line that is still blamed to this commit), going one commit up, and looking at blame of the file you're interested in then.

Compare that with looking at the blame of a file that's riddled with commits that change formatting around a semantic change, too. They will necessarily touch unrelated lines immediately around the semantic change, poisoning the visible history without a single revision that you can step over.

Rewriting code is inherently dangerous.

As Carl says, it's not. I reformatted tens of millions of lines of code with it by now. I could only confidently do this because of the sanity checks that Black performs.

@gaborbernat
Copy link
Contributor Author

Will not fix it is then 👍

@wbolster
Copy link
Contributor

wbolster commented Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: documentation Improvements to the docs (e.g. new topic, correction, etc)
Projects
None yet
Development

No branches or pull requests

6 participants