taplo icon indicating copy to clipboard operation
taplo copied to clipboard

`format --diff` still overwrites file

Open dtolnay opened this issue 2 years ago • 2 comments

I have the following TOML file:

# thing.toml

space="none"

Running taplo format --diff thing.toml produces the following output on stdout, as expected (modulo #415):

diff a//git/repro/taplo/thing.toml b//git/repro/taplo/thing.toml
--- a//git/repro/taplo/thing.toml
+++ b//git/repro/taplo/thing.toml
# thing.toml

@@ -0,2 +0,2 @@
@@ -2,1 +2,1 @@
-space="none"
+space = "none"

However, it also overwrites thing.toml with the resulting formatted toml. This is unexpected. Other tools' equivalent option for diff generation only prints the diff, leaving the original file in place.

For example rustfmt:

$ cat repro.rs
// repro.rs

fn main ( ) { }
$ rustfmt --check repro.rs
Diff in /git/repro/taplo/repro.rs at line 1:
 // repro.rs
 
-fn main ( ) { }
+fn main() {}
 
$ cat repro.rs
// repro.rs

fn main ( ) { }

dtolnay avatar Jun 02 '23 23:06 dtolnay

As a workaround I found taplo format --check --diff thing.toml.

I think not overwriting files would be the better behavior for --diff, even without --check.

That said, I am not clear what the intended use of --diff is. A hypothetical usage I was able to think of is:

# .github/workflows/ci.yml

...
  steps:
     ...
    - run: cargo test
    - name: "check that source files are properly formatted"
      run: cargo fmt -- --check
      if: always()
    - name: "the toml files too"
      run: taplo format --diff && git diff --exit-code >/dev/null
      if: always()

Here, what the user wants (from both rustfmt and taplo) is:

  1. If any files in the repo are incorrectly formatted, fail CI on the commit.

  2. Show the lines that need to be reformatted, not just the filenames. This would be important if the user was not able to, or did not want to, install rustfmt/taplo on their machine when writing their contribution to someone else's repo, or had a different rustfmt/taplo version installed than required by CI. They can manually apply the required formatting.

But for this objective, taplo format --diff is not any better than taplo format && git diff --exit-code.

Are there any foreseen use cases for taplo format --diff without --check? If not, perhaps --diff should imply --check. Separately, perhaps --check should imply --diff, like in rustfmt? At that point it would be okay to remove --diff altogether.

dtolnay avatar Jun 03 '23 00:06 dtolnay

I agree with you. I also don't understand the use of taplo format --diff and decided to not use it and instead do the same as you suggest, see here.

I'm not sure how best to proceed here. What are the options?

  • Do nothing (given I'm only an interim maintainer, I would prefer this option)
  • Remove --diff
  • Make --diff imply --check

ia0 avatar Jun 26 '23 22:06 ia0