Add a new "modified lines" write mode. #2086

jugglerchris · 2017-10-24T22:51:03Z

This is work towards #1173. I've added a new Modified write mode (not convinced it's a good name; very happy to change to a better one, especially as it's user-exposed).

This builds on top of #2070 (make make_diff work with zero context).

Some concerns I have:

The name of the write mode (Modified) is not very clear; maybe UpdatedLines or something like that?
The formatting of the output, especially the header line.
- Perhaps it should be a very simple "123,4,5" format so that it would be a reasonably machine readable format to feed to the RLS?
I think the RLS (where this feature is actually wanted AIUI) probably still wants a modified format_input function to get the result directly rather than as text.

nrc · 2017-10-26T23:51:01Z

Thanks for the PR! It's a bit tricky to tell exactly what is going on because this is based on #2070.

From a high level, while I think implementing this as a write mode internally is probably right, I don't think it should be user-facing and thus doesn't need a text version of the output.

I'd hope you could provide a function which would call format_input and then post-process the result to make the 'modified lines' data structure.

jugglerchris · 2017-10-27T21:46:01Z

As far as being based on the other PR I was hoping Github would make those changes disappear from here if that was merged, but looks like it doesn't, sorry!

I'll rework based on your feedback (which all makes sense!), thanks!

jugglerchris · 2017-11-20T02:02:59Z

I've had a bit more time to play with this; there's now a function imaginatively called get_modified_lines which returns a type giving the updated lines. It uses format_input internally with an internal write mode which does some simple serialisation (of line numbers etc.) into the out stream.

I'd appreciate some feedback (when convenient; I have kids too!) on whether this is the right direction. Also (either way) whether you'd prefer a new branch or for me to just fix this one. :-)

nrc

A couple of comments inline, otherwise looks great! Could you squash the commits please?

nrc · 2017-11-20T04:21:59Z

src/lib.rs

+pub fn get_modified_lines(
+    input: Input,
+    config: &Config,
+) -> Result<(Summary, FileMap, FormatReport, ModifiedLines), (io::Error, Summary)> {


Could you return a struct rather than a tuple please? It is more self-documenting.

Done (feel free to suggest a better name).

nrc · 2017-11-20T04:23:19Z

src/rustfmt_diff.rs

+///     lineno num_removed num_added
+/// followd by (num_added) lines of added text.  The line numbers are
+/// relative to the original file.
+pub fn output_modified<W>(mut out: W, diff: Vec<Mismatch>)


Do we need this function? I thought you removed the facility to write out the 'modified' output

Well, it's used to serialise the result (really just line count header lines before the changed lines) into the out passed into write_file. The alternatives (either of which I'm happy to do) are adding another parameter to write_file or coming up with another similar function which had something else instead of the out: &mut T to return the modified lines more directly.

nrc · 2017-11-20T04:24:35Z

And it looks like the code is not compiling now.

changed lines from rustfmting. Squashed commit of the following: commit e90f9da Author: Chris Emerson <[email protected]> Date: Sat Jan 20 20:10:16 2018 +0000 Fix tests after merging with master. commit c3af004 Merge: 0386858 e0e3e22 Author: Chris Emerson <[email protected]> Date: Sat Jan 20 17:45:05 2018 +0000 Merge remote-tracking branch 'origin/master' into HEAD commit 0386858 Author: Chris Emerson <[email protected]> Date: Mon Nov 20 01:57:56 2017 +0000 Fix some warnings. commit 162b134 Author: Chris Emerson <[email protected]> Date: Mon Nov 20 01:48:02 2017 +0000 Remove unneeded import. commit 20cce3c Merge: 81e9814 fa794f5 Author: Chris Emerson <[email protected]> Date: Mon Nov 20 01:07:17 2017 +0000 Merge branch 'master' into difflines_mode commit 81e9814 Author: Chris Emerson <[email protected]> Date: Mon Nov 20 01:02:50 2017 +0000 Add a simple "modified lines" test. commit 018390c Author: Chris Emerson <[email protected]> Date: Thu Nov 2 23:06:21 2017 +0000 Update test output. commit 7909f49 Author: Chris Emerson <[email protected]> Date: Thu Nov 2 23:03:22 2017 +0000 Rerun rustfmt. commit 6275f1a Merge: 7a66d28 175c0c6 Author: Chris Emerson <[email protected]> Date: Thu Nov 2 21:40:29 2017 +0000 Merge remote-tracking branch 'origin/master' into difflines_mode commit 7a66d28 Author: Chris Emerson <[email protected]> Date: Thu Nov 2 21:36:40 2017 +0000 WIP: Add a separate API to get changed lines. Currently calls format_input() and adjusts the output. commit c8163a9 Author: Chris Emerson <[email protected]> Date: Fri Oct 27 22:53:33 2017 +0100 Remove "modified" from the documentation again. commit 94041fa Merge: acaa3c7 2adf7ee Author: Chris Emerson <[email protected]> Date: Fri Oct 27 22:47:05 2017 +0100 Merge branch 'master' into difflines_mode commit acaa3c7 Author: Chris Emerson <[email protected]> Date: Tue Oct 24 23:34:14 2017 +0100 Update the Modified write mode to use `out` instead of just prinln!(). This means we can test it more easily, so do so. commit 9f1bbca Author: Chris Emerson <[email protected]> Date: Tue Oct 24 23:11:55 2017 +0100 Add "Modified" to the various lists of modes. commit e12f023 Author: Chris Emerson <[email protected]> Date: Tue Oct 24 22:57:33 2017 +0100 Rerun cargo fmt. commit 0f8a436 Author: Chris Emerson <[email protected]> Date: Tue Oct 24 22:46:26 2017 +0100 Add `line_number_orig` to instances of `Mismatch` in tests. commit d432a70 Author: Chris Emerson <[email protected]> Date: Tue Oct 24 22:41:40 2017 +0100 Add a `line_number_orig` field to `Mismatch` to track the pre-format line number. Use that for the write-mode=modified output. commit bdb7d1d Author: Chris Emerson <[email protected]> Date: Tue Oct 24 22:35:50 2017 +0100 First basic --write-mode=modified implementation. commit ea1433d Author: Chris Emerson <[email protected]> Date: Fri Oct 20 00:04:16 2017 +0100 WIP on new "modified" mode. commit 27ee948 Merge: e48dd81 2a84352 Author: Chris Emerson <[email protected]> Date: Tue Oct 24 21:56:44 2017 +0100 Merge remote-tracking branch 'jc/diff_zero_context' into difflines_mode

jugglerchris · 2018-01-20T20:53:35Z

So, two months later (oops!) I've now made the suggested updates. Let me know if you want me to do anything else, and I'll try to be slightly quicker next time!

nrc · 2018-01-29T04:15:26Z

Configurations.md

@@ -1766,7 +1766,7 @@ See also: [`match_block_trailing_comma`](#match_block_trailing_comma).
 What Write Mode to use when none is supplied: Replace, Overwrite, Display, Diff, Coverage

 - **Default value**: `"Overwrite"`
- **Possible values**: `"Checkstyle"`, `"Coverage"`, `"Diff"`, `"Display"`, `"Overwrite"`, `"Plain"`, `"Replace"`
+- **Possible values**: `"Checkstyle"`, `"Coverage"`, `"Diff"`, `"Display"`, `"Overwrite"`, `"Plain"`, `"Replace"`, `"Modified"`


Given this is not meant to be a user-facing option, it shouldn't be documented here.

nrc · 2018-01-29T04:15:44Z

src/config.rs

@@ -128,6 +128,8 @@ configuration_option_enum! { WriteMode:
    Plain,
    // Outputs a checkstyle XML file.
    Checkstyle,
+    // Output the changed lines
+    Modified,


Could you note that this option is only meant to be used via an API

nrc · 2018-01-29T04:16:12Z

src/config.rs

@@ -677,7 +679,7 @@ create_config! {
    // Control options (changes the operation of rustfmt, rather than the formatting)
    write_mode: WriteMode, WriteMode::Overwrite, false,
        "What Write Mode to use when none is supplied: \
-         Replace, Overwrite, Display, Plain, Diff, Coverage";
+         Replace, Overwrite, Display, Plain, Diff, Coverage, Modified";


Shouldn't be here, iiuc

nrc · 2018-01-29T04:18:49Z

src/lib.rs

+#[derive(Debug, PartialEq, Eq)]
+pub struct ModifiedChunk {
+    /// The first affected line before formatting.
+    pub line_number: u32,


To correspond with Mismatch, this should be line_number_orig, the comment might be better phrased as "The first line to be removed from the original text"

That sounds like better wording, thanks!

nrc · 2018-01-29T04:20:23Z

src/lib.rs

+        summary: summary,
+        filemap: filemap,
+        report: formatreport,
+        modified_lines: ModifiedLines { chunks: chunks },


nit: s/chunks: chunks/chunks

nrc · 2018-01-29T04:24:24Z

src/filemap.rs

@@ -167,6 +167,15 @@ where
                return Ok(has_diff);
            }
        }
+        WriteMode::Modified => {
+            let filename = filename_to_path();


I think we can just do nothing here, or just do the has_diff part and then remove output_modified since I think the important bit of this work is returning the info from get_modified_lines rather than writing something out as in other write modes.

Writing to out here is needed as that's how get_modified_lines gets its results (it's passed down via format_input), as there's no other way (as format_input exists at the moment) to pass the results back other than through this T: Write.

I did look initially at factoring the guts of format_input out in the hope of sharing that with get_modified_lines but it seemed hard to avoid ending up with two fairly long and very similar functions. I can look again if you think that's worth it.

nrc · 2018-02-15T02:21:50Z

Thank you!

nrc reviewed Nov 20, 2017

View reviewed changes

jugglerchris force-pushed the difflines_mode branch from e90f9da to fc377f3 Compare January 20, 2018 20:28

jugglerchris added 2 commits January 20, 2018 20:45

Return a struct instead of a 4-tuple from get_modified_lines().

7213b88

Fix typo in comment.

6f66909

nrc reviewed Jan 29, 2018

View reviewed changes

jugglerchris added 4 commits February 12, 2018 22:43

Merge remote-tracking branch 'origin/master' into difflines_mode

c2fda14

Move the modified test files to the new location.

e02f268

Update after review comments.

807f292

Update comment to say that Modified is for internal use only.

2a3caa7

nrc merged commit 091ed2f into rust-lang:master Feb 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new "modified lines" write mode. #2086

Add a new "modified lines" write mode. #2086

jugglerchris commented Oct 24, 2017

nrc commented Oct 26, 2017

jugglerchris commented Oct 27, 2017

jugglerchris commented Nov 20, 2017

nrc left a comment

nrc Nov 20, 2017

jugglerchris Jan 20, 2018

nrc Nov 20, 2017

jugglerchris Jan 20, 2018

nrc commented Nov 20, 2017

jugglerchris commented Jan 20, 2018

nrc Jan 29, 2018

jugglerchris Feb 12, 2018

nrc Jan 29, 2018

jugglerchris Feb 12, 2018

nrc Jan 29, 2018

jugglerchris Feb 12, 2018

nrc Jan 29, 2018

jugglerchris Feb 12, 2018

nrc Jan 29, 2018

jugglerchris Feb 12, 2018

nrc Jan 29, 2018

jugglerchris Feb 12, 2018

nrc commented Feb 15, 2018

Add a new "modified lines" write mode. #2086

Add a new "modified lines" write mode. #2086

Conversation

jugglerchris commented Oct 24, 2017

nrc commented Oct 26, 2017

jugglerchris commented Oct 27, 2017

jugglerchris commented Nov 20, 2017

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Nov 20, 2017

jugglerchris commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Feb 15, 2018