Add `--diff` option `ruff format` #7937

konstin · 2023-10-13T09:31:30Z

Summary ruff format --diff is similar to ruff format --check, but we don't only error with the list of file that would be formatted, but also show a diff between the unformatted input and the formatted output.

$ ruff format --diff scratch.py scratch.pyi scratch.ipynb
warning: `ruff format` is not yet stable, and subject to change in future versions.
--- scratch.ipynb
+++ scratch.ipynb
@@ -1,3 +1,4 @@
 import numpy
-maths = (numpy.arange(100)**2).sum()
-stats= numpy.asarray([1,2,3,4]).median()
+
+maths = (numpy.arange(100) ** 2).sum()
+stats = numpy.asarray([1, 2, 3, 4]).median()
--- scratch.py
+++ scratch.py
@@ -1,3 +1,3 @@
 x = 1
-y=2
+y = 2
 z = 3
2 files would be reformatted, 1 file left unchanged

With --diff, the summary message gets printed to stderr to allow e.g. ruff format --diff . > format.patch.

At the moment, jupyter notebooks are formatted as code diffs, while everything else is a real diff that could be applied. This means that the diffs containing jupyter notebooks are not real diffs and can't be applied. We could change this to json diffs, but they are hard to read. We could also split the diff option into a human diff option, where we deviate from the machine readable diff constraints, and a proper machine readable, appliable diff output that you can pipe into other tools.

To make the tests work, the results (and errors, if any) are sorted before printing them. Previously, the print order was random, i.e. two identical runs could have different output.

Open question: Should this go into the markdown docs? Or will this be subsumed by the integration of the formatter into ruff check?

Test plan Fixtures for the change and no change cases, including a jupyter notebook and for file input and stdin.

Fixes #7231

github-actions · 2023-10-13T10:39:43Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

crates/ruff_cli/src/commands/format.rs

charliermarsh · 2023-10-13T19:59:49Z

crates/ruff_cli/src/commands/format.rs

+    Formatted {
+        unformatted: SourceKind,
+        formatted: SourceKind,
+    },


This is making me wonder if format_source should take &SourceKind, and we should change Unchanged(SourceKind) to just Unchanged, and remove unformatted from the Formatted variant. It strikes me as inflexible that the method takes ownership of SourceKind but only acts on references and then returns the owned SourceKind in all cases. But, this is minor -- if it doesn't work for whatever reason, or you disagree, it's fine.

I guess the challenge with this is that we need to move SourceKind out of the linter, maybe into ruff_source_file? I recommend investigating this as a separate PR.

Thought so too but didn't want to create so much churn, changed it now

charliermarsh · 2023-10-13T20:00:31Z

Should this go into the markdown docs?

I think it's fine to leave this as-is for now -- I will fold into the documentation when I revisit it next week.

crates/ruff_cli/src/commands/format.rs

MichaReiser · 2023-10-16T02:01:07Z

crates/ruff_cli/src/commands/format.rs

+    Formatted {
+        unformatted: SourceKind,
+        formatted: SourceKind,
+    },


I guess the challenge with this is that we need to move SourceKind out of the linter, maybe into ruff_source_file? I recommend investigating this as a separate PR.

crates/ruff_cli/src/commands/format.rs

MichaReiser · 2023-10-16T02:04:42Z

crates/ruff_cli/src/commands/format.rs

+                    let mut writer = stdout().lock();
+                    let text_diff = TextDiff::from_lines(
+                        unformatted.source_code(),
+                        formatted_source.source_code(),
+                    );
+                    let mut unified_diff = text_diff.unified_diff();
+                    unified_diff.header(&fs::relativize_path(path), &fs::relativize_path(path));
+                    unified_diff.to_writer(&mut writer).unwrap();
+
+                    formatted += 1;
+                }


Nit: It feels a bit strange that the diff is written as part of the FormatSummary's Display implementation (the diff isn't part of the summary).

I would prefer moving the diff printing out of the summary implementation and handle it explicitly when the FormatMode is Diff (also allows to locking stdout exactly once)

I went with renaming FormatSummary to FormatResults and giving it two different write methods. We unfortunately lose the fmt impl but we get proper separation

konstin · 2023-10-16T13:25:28Z

I guess the challenge with this is that we need to move SourceKind out of the linter, maybe into ruff_source_file? I recommend investigating this as a separate PR.

Did work without it, but i'm fine with moving it anyway

**Summary** `ruff format --diff` is similar to `ruff format --check`, but we don't only error with the list of file that would be formatted, but also show a diff between the unformatted input and the formatted output. ```console $ ruff format --diff scratch.py scratch.pyi scratch.ipynb warning: `ruff format` is not yet stable, and subject to change in future versions. --- scratch.ipynb +++ scratch.ipynb @@ -1,3 +1,4 @@ import numpy -maths = (numpy.arange(100)**2).sum() -stats= numpy.asarray([1,2,3,4]).median() + +maths = (numpy.arange(100) ** 2).sum() +stats = numpy.asarray([1, 2, 3, 4]).median() --- scratch.py +++ scratch.py @@ -1,3 +1,3 @@ x = 1 -y=2 +y = 2 z = 3 2 files would be reformatted, 1 file left unchanged ``` With `--diff`, the summary message gets printed to stderr to allow e.g. `ruff format --diff . > format.patch`. At the moment, jupyter notebooks are formatted as code diffs, while everything else is a real diff that could be applied. This means that the diffs containing jupyter notebooks are not real diffs and can't be applied. We could change this to json diffs, but they are hard to read. We could also split the diff option into a human diff option, where we deviate from the machine readable diff constraints, and a proper machine readable, appliable diff output that you can pipe into other tools. **Test plan** Fixtures for the change and no change cases, including a jupyter notebook and for file input and stdin.

Co-authored-by: Micha Reiser <[email protected]>

konstin added the formatter Related to the formatter label Oct 13, 2023

charliermarsh reviewed Oct 13, 2023

View reviewed changes

crates/ruff_cli/src/commands/format.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Oct 13, 2023

View reviewed changes

crates/ruff_cli/src/commands/format.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Oct 13, 2023

View reviewed changes

crates/ruff_cli/src/commands/format.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Oct 13, 2023

View reviewed changes

charliermarsh approved these changes Oct 13, 2023

View reviewed changes

MichaReiser added the cli Related to the command-line interface label Oct 16, 2023

MichaReiser approved these changes Oct 16, 2023

View reviewed changes

konstin force-pushed the format-diff branch from 61f4914 to e15a7ba Compare October 16, 2023 13:01

konstin and others added 9 commits October 18, 2023 13:40

Fix windows paths

3d2a088

Make ruff format output deterministic

5ded36e

Review (charlie)

375d1d1

Update crates/ruff_cli/src/commands/format.rs

68ee65c

Co-authored-by: Micha Reiser <[email protected]>

Update crates/ruff_cli/src/commands/format.rs

896a31b

Co-authored-by: Micha Reiser <[email protected]>

Review (micha)

f9cfc32

FormatSummary -> FormatResults

81b43ba

Fix missing newline

9e14fe7

konstin force-pushed the format-diff branch from 00ddc96 to 9e14fe7 Compare October 18, 2023 11:43

Rebase fallout

8daf970

konstin enabled auto-merge (squash) October 18, 2023 11:45

konstin merged commit 51aa73f into main Oct 18, 2023
15 checks passed

konstin deleted the format-diff branch October 18, 2023 11:55

p-linnane mentioned this pull request Oct 19, 2023

ruff 0.1.1 Homebrew/homebrew-core#151871

Merged

jhkennedy mentioned this pull request Nov 30, 2023

Switch to Ruff nsidc/earthaccess#372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--diff` option `ruff format` #7937

Add `--diff` option `ruff format` #7937

konstin commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited

Loading

charliermarsh Oct 13, 2023

MichaReiser Oct 16, 2023

konstin Oct 16, 2023

charliermarsh commented Oct 13, 2023

MichaReiser Oct 16, 2023

MichaReiser Oct 16, 2023

konstin Oct 16, 2023

konstin commented Oct 16, 2023

Add --diff option ruff format #7937

Add --diff option ruff format #7937

Conversation

konstin commented Oct 13, 2023 • edited Loading

github-actions bot commented Oct 13, 2023 • edited Loading

PR Check Results

Ecosystem

charliermarsh Oct 13, 2023

Choose a reason for hiding this comment

MichaReiser Oct 16, 2023

Choose a reason for hiding this comment

konstin Oct 16, 2023

Choose a reason for hiding this comment

charliermarsh commented Oct 13, 2023

MichaReiser Oct 16, 2023

Choose a reason for hiding this comment

MichaReiser Oct 16, 2023

Choose a reason for hiding this comment

konstin Oct 16, 2023

Choose a reason for hiding this comment

konstin commented Oct 16, 2023

Add `--diff` option `ruff format` #7937

Add `--diff` option `ruff format` #7937

konstin commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited

Loading