Add option to show line numbers #190

clnoll · 2020-05-14T14:21:21Z

Adds a CLI option, --number, that shows line numbers for the input and output diffs. The first column shows the line number for the original file, and the second column shows line number for the output file. A blank cell indicates that the line does not exist in one of the files (was removed or added, respectively).

Also provides the ability to customize the appearance of line numbers. Users can pass in a format string showing the position of the line numbers within the column, and can apply style elements to the numbers as well as the format string text, using new command line options:

--number-minus-style
--number-plus-style
--number-minus-format
--number-plus-format
--number-minus-format-style
--number-plus-format-style

Addresses #130.

TODO: tests.

dandavison · 2020-05-14T14:42:11Z

Thanks very much for this @clnoll! This is a substantial contribution and it's a feature that's been requested multiple times. I'll start reviewing.

src/cli.rs

dandavison · 2020-05-14T15:07:30Z

src/delta.rs

-    let (raw_code_fragment, line_number) = parse::parse_hunk_metadata(&line);
+    let (raw_code_fragment, line_number_in, line_number_out) = parse::parse_hunk_metadata(&line);
+    painter.line_number_in = line_number_in.parse::<usize>().unwrap_or(0);
+    painter.line_number_out = line_number_out.parse::<usize>().unwrap_or(0);


Do you know under what circumstances we expect this parse to fail?

It was failing on new files (new line number would be 0). But at any rate as you've seen, I updated parse_hunk_metadata to handle this case.

src/paint.rs

src/delta.rs

dandavison · 2020-05-14T16:47:28Z

src/delta.rs

+                None,
+                Some(painter.line_number_out),
+                config,
+            ));


Are these ones (in the - and + branches) necessary?

Nope! Removed.

src/paint.rs

dandavison · 2020-05-14T17:23:20Z

src/delta.rs

+    let divider = paint::paint_text_foreground("│", config.hunk_color, config.true_color);
+    format!("{}{}", numbers, divider)
+}
+


Playing around with the vertical dividing line character. E.g. https://jrgraphix.net/r/Unicode/2500-257F https://unicode.org/charts/nameslist/n_2500.html. Any thoughts? (I haven't found one yet which yields a vertical dashed line without "breaks" between lines.)

Current:

Alternatives:

U2506

U250A

Added an option to input a divider (--number-center-divider-string).

rygwdn · 2020-05-15T23:37:49Z

src/delta.rs

+        Some(x) => format!("{:^4}", x),
+        None => format!("    "),


does this handle line numbers over 4 digits without breaking alignment? Maybe if it were deferred to paint_lines as suggested below, it would be easier to ensure that longer numbers are handled since you'd have access to all of them.

👍 deferred to paint_lines. Need to take a look at how it handles >4 digits.

clnoll · 2020-05-16T02:25:44Z

Thanks @dandavison & @rygwdn! Great suggestions, making these changes.

clnoll · 2020-05-17T03:10:27Z

@dandavison @rygwdn I took all of the suggestions mentioned above, and added some additional command line options (colors for the line numbers and the dividers, and the ability to configure a string for the dividers).

FYI I'm seeing that it's a bit slower than master when benchmarking, so will take a closer look to see what I can do to speed it up.

Master:

Time (mean ± σ):      1.496 s ±  0.060 s    [User: 1.494 s, System: 0.042 s]
Range (min … max):    1.460 s …  1.661 s    10 runs

Feature branch, with --numbers

Time (mean ± σ):      1.807 s ±  0.078 s    [User: 1.786 s, System: 0.044 s]
Range (min … max):    1.757 s …  2.019 s    10 runs

Feature branch, without --numbers

Time (mean ± σ):      1.722 s ±  0.017 s    [User: 1.720 s, System: 0.043 s]
Range (min … max):    1.696 s …  1.748 s    10 runs

dandavison

OK this is looking fantastic. Delta is getting much more customizable! In addition to having line numbers, with the configurability you've added here it's going to be possible to use it in some interesting new ways I think. I've made a few minor comments and will now have a look at performance etc.

src/cli.rs

dandavison · 2020-05-17T15:23:36Z

README.md

@@ -219,6 +219,7 @@ FLAGS:
                                     The default behavior is to output a space character in place of these markers.
        --light                      Use default colors appropriate for a light terminal background. For more control,
                                     see the other color options.
+        --line-numbers               Show line numbers for each line of the diff hunk.


This is out of date now. (I do this step with delta --help >> README.md and then moving the appended text into position and fiddling around a bit.)

src/config.rs

src/cli.rs

dandavison · 2020-05-18T14:20:00Z

src/parse.rs

+    let caps = re.captures(line).unwrap();
+    let line_numbers = _make_line_number_vec(caps.name("lns").unwrap().as_str());
+    let code_fragment = caps.name("cf").unwrap().as_str();
+    return (code_fragment, line_numbers);


I just learned that building regular expressions like this inside a function called many times is extremely inefficient. This commit on master speeds delta up by almost 2x 2cb3e40! I suspect if you do the same thing here it will get rid of the performance difference between this branch and master.

In addition to make benchmark I've also added make flamegraph, for profiling on MacOS, which is sort of fun. (The Makefile assumes that you've cloned https://github.com/brendangregg/FlameGraph and symlinked two of its perl scripts into your $PATH with the names flamegraph and stackcollapse-sample.)

Awesome, updated to do the same.

dandavison · 2020-05-18T14:23:03Z

src/parse.rs

+    }
+    return numbers;
+}
+


Do you think it's possible to do it all with a regular expression, i.e. capture the numbers as well rather than using this function? While we're at it, we could also capture and expose the first set of @ markers seeing as they will be needed for determining the number of merge parents for #189 cc @rygwdn

dandavison · 2020-05-23T14:02:58Z

I've been doing a lot of refactoring in parallel and so if it's OK with you ~~I'd like to treat style-strings as the base branch for this branch to target~~ EDIT: the refactoring is all in master now. Seeing as you've already had to rebase once, I rebased this branch on top of style-strings as line-numbers-rebased. If that diff LGTY then please feel free to take it (the commit has your attribution as author email).

You'll note that it...doesn't actually compile :) But it's fairly superficial -- we need to re-express the color options in this branch as what are referred to as "styles" in style-strings. The function paint_text_foreground has gone away and one just calls style.paint(...) instead.

dandavison · 2020-05-23T14:13:05Z

src/cli.rs

+    /// Color for the right (plus) column of line numbers (--number), if --number is set.
+    /// Defaults to --hunk-color.
+    #[structopt(long = "number-plus-foreground-color")]
+    pub number_plus_foreground_color: Option<String>,


In the new world of the style-strings branch I think we're going to want these options to be named --number-minus-style and --number-plus-style. (It might help to take a look at the new style documentation in delta --help.)

dandavison · 2020-05-23T14:20:17Z

src/cli.rs

+
+    /// String to use as the right divider of the line numbers section, if --number is set.
+    #[structopt(long = "number-right-divider-string", default_value = "│")]
+    pub number_right_divider_string: String,


I think I'd like to investigate offering users a format string, rather than specifying the divider characters individually. So basically something with placeholder slots for the two line numbers. It's hard to make future-proof decisions about the CLI: I wonder whether that should be called something like --hunk-line-format in order to retain the possibility that in the future it might have other placeholder slots?

👍 made this change.

dandavison · 2020-05-26T15:51:16Z

I've been doing a lot of refactoring in parallel and so if it's OK with you I'd like to treat style-strings as the base branch for this branch to target

The style-string refactoring is all in master now.

clnoll

@dandavison I've addressed all of your comments and modified the CLI to accept a format string & the updated style strings. Let me know what you think!

dandavison · 2020-06-09T02:31:43Z

src/parse.rs

+        assert_eq!(code_fragment, " dependencies =",);
+        assert_eq!(line_numbers[0], 293,);
+        assert_eq!(line_numbers[1], 358,);
+        assert_eq!(line_numbers[2], 358,);


Nice! Looks like this will help with highlighting merge diffs in #189.

- format string for specifying minus number line - format string for specifying plus number line - minus number style - plus number style - minus format string style - plus format string style

dandavison

Thanks very much for this work @cnoll. This is a really nicely implemented and substantial feature, that I think people are going to appreciate, and with tons of customizability. Merging now!

clnoll force-pushed the line-numbers branch 3 times, most recently from 92fe139 to aa1b35b Compare May 14, 2020 15:14

dandavison reviewed May 14, 2020

View reviewed changes

src/paint.rs Outdated Show resolved Hide resolved

dandavison reviewed May 14, 2020

View reviewed changes

src/delta.rs Outdated Show resolved Hide resolved

dandavison reviewed May 14, 2020

View reviewed changes

src/paint.rs Outdated Show resolved Hide resolved

dandavison reviewed May 14, 2020

View reviewed changes

dandavison mentioned this pull request May 15, 2020

🚀 Highlight merge diffs and conflict markers #189

Closed

rygwdn reviewed May 15, 2020

View reviewed changes

clnoll force-pushed the line-numbers branch 2 times, most recently from 70730d7 to 487649b Compare May 17, 2020 03:10

clnoll force-pushed the line-numbers branch 2 times, most recently from fab512b to a07c8f2 Compare May 17, 2020 15:21

dandavison reviewed May 17, 2020

View reviewed changes

clnoll force-pushed the line-numbers branch 3 times, most recently from da2780b to c62d67c Compare May 18, 2020 01:24

dandavison reviewed May 18, 2020

View reviewed changes

dandavison reviewed May 23, 2020

View reviewed changes

clnoll force-pushed the line-numbers branch 2 times, most recently from 037846f to ba30656 Compare June 7, 2020 19:02

clnoll force-pushed the line-numbers branch 8 times, most recently from ab02c14 to 030a997 Compare June 8, 2020 03:36

clnoll commented Jun 8, 2020

View reviewed changes

clnoll force-pushed the line-numbers branch 2 times, most recently from 19f34f2 to 142b1d0 Compare June 9, 2020 01:51

dandavison reviewed Jun 9, 2020

View reviewed changes

Add --number option for showing line numbers. Also adds related options:

d5ca6d9

- format string for specifying minus number line - format string for specifying plus number line - minus number style - plus number style - minus format string style - plus format string style

clnoll force-pushed the line-numbers branch from 142b1d0 to d5ca6d9 Compare June 9, 2020 02:44

dandavison approved these changes Jun 9, 2020

View reviewed changes

dandavison merged commit aa83d41 into dandavison:master Jun 9, 2020

dandavison mentioned this pull request Jun 10, 2020

Command line option to apply line number style to unchanged lines #219

Merged

dandavison mentioned this pull request Jun 17, 2020

Side by side viewer #86

Closed

This was referenced Jul 1, 2020

Time to cut a new release? #223

Closed

🚀RFC: New color/style options, diff-so-fancy/diff-highlight emulation #205

Closed

Would it be possible to add a line number sidebar? #130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to show line numbers #190

Add option to show line numbers #190

clnoll commented May 14, 2020 •

edited

Loading

dandavison commented May 14, 2020

dandavison May 14, 2020

clnoll May 17, 2020

dandavison May 14, 2020

clnoll May 17, 2020

dandavison May 14, 2020

clnoll May 17, 2020

rygwdn May 15, 2020

clnoll May 17, 2020

clnoll commented May 16, 2020

clnoll commented May 17, 2020

dandavison left a comment

dandavison May 17, 2020

clnoll May 17, 2020

dandavison May 18, 2020

clnoll Jun 7, 2020

dandavison May 18, 2020

dandavison commented May 23, 2020 •

edited

Loading

dandavison May 23, 2020

clnoll Jun 7, 2020

dandavison May 23, 2020

clnoll Jun 7, 2020

dandavison commented May 26, 2020

clnoll left a comment

dandavison Jun 9, 2020

dandavison left a comment

Add option to show line numbers #190

Add option to show line numbers #190

Conversation

clnoll commented May 14, 2020 • edited Loading

dandavison commented May 14, 2020

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

clnoll commented May 16, 2020

clnoll commented May 17, 2020

dandavison 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

Choose a reason for hiding this comment

dandavison commented May 23, 2020 • edited Loading

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

dandavison commented May 26, 2020

clnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison left a comment

Choose a reason for hiding this comment

clnoll commented May 14, 2020 •

edited

Loading

dandavison commented May 23, 2020 •

edited

Loading