black icon indicating copy to clipboard operation
black copied to clipboard

FR: Option for outputting the diff + applying the changes.

Open kxrob opened this issue 2 years ago • 1 comments

I want to immediately see on the output (stdout) what has changed after a black run. --diff stops writing back the file. So it would require an arduous tee-ing to a temp file and risky patch ; or running black twice; or commit before black, git diff, and ammend

It could be done by:

  • double verbose: -vv
  • --diff+
  • --diff --write ...

kxrob avatar Dec 10 '22 16:12 kxrob

I'm inclined to say this is too niche to add a new feature, but willing to reconsider if more users are interested.

JelleZijlstra avatar Dec 20 '22 02:12 JelleZijlstra

@JelleZijlstra I'm not sure that the proposed solution is the right one, but there is clearly a need here.

The root problem is IMO that once you reformatted in place, you lose the diff, or, if you asked for the diff, you end up in the precarious position to apply the patch yourself, which have all kind of failure scenarii one would rather avoid.

Instead of multiplying the option to generate the diff and edit the file and whatnot, which doesn't strike me as a good path forward as you enter the world of combinatorial configuration flags explosion, I would propose an alternative: add a flag, such as -o to tell black where the put the formatted output. This is a very common way of doing this, and typically would also accept - to dump the output on stdout.

This way one can get what OP want with very little overhead:

  • The original file isn't modified.
  • It's easy to generate a diff if need be.
  • It's easy to apply the changes if need be.

Now, maybe this is what i should have started with, but why would one want that? Well, long story short: to integrate black into existing toolchains. For instance, we have a codebase with C/C++, Javascript and Python in it. We use prettier for JS, and clang-format for C/C++. We use some tool to submit the changes for review. Before putting something up for review, the tool run the formatters, if the files are modified, it prompts the users about it, and the user can chose to apply the change, abort the process, and ignore and proceed. Integrating black in that workflow, with its current API, proves challenging, where prettier or clang-format was trivial.

Is this something that could be considered? That would be tremendously helpful, and I think that would solve @kxrob 's problem as well.

deadalnix avatar May 15 '23 21:05 deadalnix

I wouldn't be opposed to such a flag if there's a clear need, but adding such an option does add maintenance cost for us, so a few counterpoints:

  • We generally recommend running Black as early as possible and without further review (our AST safety checks make that safe). For example, there are many tools to run Black on save in your editor. In your workflow, the tool shouldn't need to prompt the user: it should just apply Black. If you have a reason why that doesn't work for you, I'd be curious to hear it.
  • What you want could be done by running black --diff and then using something like patch to apply the diff. Sure, it's a little roundabout, but isn't the diff format sufficiently standardized that this should work?

JelleZijlstra avatar May 15 '23 21:05 JelleZijlstra

I admittedly did not look at the implementation, so i might be wrong, but I'd expect the --diff flag to be implemented as follow:

  1. Run the formatting algorithm on the code and output the result in memory.
  2. Run a diff algorithm between the original content of the file and the formatted version of the same code.
  3. Dump the resulting diff on stdout.

What's really necessary here is to be able to skip step 2. This doesn't really require to do anything that black doesn't already does, as far as I'm aware, to the contrary, it would be a simplification of what it already does (unfortunately, this simplification is unlikely to translate into much simplification in the code).

You are correct that the workflow could be done leveraging the --diff flag, but that entail a number of problems, the standardization of the diff format being one of them, but probably not the most important one. To begin with, that would entail to make sure all users have a version of patch available in their path. That seems dumb, but this is likely not the case for a lot of windows users. Another one would be bad UI for people who use features such as git's 'core.autocrlf' . Diffing algorithm typically propose options that allow to ignore things such as line ending so we can present the user with what the formatter actually changed rather than a giant diff where every single line is replaced by what looks like to them as an identical line. You might argue that people should know better than have these kind of crummy setups, and to some extent I'd tend to agree with you, nevertheless, I gauge the chances of improving black's command line interface to be much better than to fix users.

But maybe more importantly, dumping the formatted output on stdout is not only a standard feature of most formatters, it is in fact the default one for most (for instance, prettier or clang-format). It's probably something inherited from the UNIX phylosophy, and, while I wouldn't put too much weight on a phylosophical argument, it is clear that this approach is the de-facto standard in the formatter space. This ensured that we could very easily integrate all kind of formatters over the years, and even swap them out easily in the toolchain (for instance, we moved from using uncrustify to use sdfmt for D code, and ti was easy).

Python is the only major language that we do not autoformat, and that's all down to the fact it presents a different integration challenge, and that, while there is no major problem with it, there are many smaller ones that make it challenging in practice.

deadalnix avatar May 16 '23 11:05 deadalnix

Side flow I use to achieve this is I generally do all my changes to the code, then git/hg commit then just run black / rustfmt etc. and use SCM's diff to see what it applied (i.e. git/hg diff. Then it's a cheap revert/reset or commit to make it useful.

I'm not saying this is a replacement/reason not to do this. Just offering the flow. I am also interested in if we should offer this, especially if other popular formatting tools do it. It seems rustfmt does not tho ... it needs --check to see the diff and is also a dry run ... I couldn't get -v or any other combination of CLI args to do what you want here - Am I missing something?

cooperlees avatar May 16 '23 15:05 cooperlees

@deadalnix another option for your use case is to use something like cat something.py | black -.

JelleZijlstra avatar May 16 '23 15:05 JelleZijlstra

@cooperlees I generally do something similar myself. Nevertheless, I'm not talking about tools we made ourselves, but off the shelf pipeline that companies use. Specifically for rustfmt, this can be done using --emit stdout.

@JelleZijlstra I did not think of that and I think that could work. Even black - < something.py if one wants to avoid creating a process for no good reason. Nevertheless, I remain convinced that block would benefit from having a way to direct the output somewhere.

deadalnix avatar May 16 '23 23:05 deadalnix

I haven't found myself too compelled by use cases mentioned here. There seem to be plenty of acceptable workarounds (use stdin, patch, just use git review, etc).

hauntsaninja avatar Jun 29 '23 06:06 hauntsaninja