Tab expansion support. #302

eth-p · 2018-09-11T02:55:29Z

This pull request adds support for expanding tabs into column-aligned spaces (similar to expand(1)). Issues #166 and #184 are fixed when using this feature.

By default, tab width is set to 8 characters. This can be overridden with --tabs=[n] or export BAT_TABS=[n]. It can also be turned off entirely by setting the value to 0.

To allow for lossless copy-pasting, when neither the pager nor the decorations are being used, the default tab width will be set to 0 (passthrough).

Implementation

The function expand (src/preprocessor.rs) is run for every line inside InteractivePrinter.print_line, after the syntax highlighting is processed. ANSI characters are directly passed through while tabs are interpreted and replaced with spaces. The number of spaces depends on the calculated cursor position, and will always try to align the columns.

Additional Information

Three new tests have been created for this feature:

tabs_4
tabs_8
tabs_passthrough

A couple of small changes were made to the tester scripts and generate_snapshots.py was fixed to use the interactive printer for generating snapshots.

Example

Before:

sidebar│         |       |       |         <-- Column Alignment
  24   │ // Tab alignment:
  25   │ /*
  26   │        Indent                     <-- Misaligned Indentation
  27   │        1       2       3       4
  28   │ 1      ?
  29   │ 22     ?
  30   │ 333    ?
  31   │ 4444   ?
  32   │ 55555  ?
  33   │ 666666 ?
  34   │ 7777777        ?                  <-- Misaligned Columns
  35   │ 88888888       ?
  36   │ */

After:

sidebar│         |       |       |
  20   │ // Tab alignment:
  21   │ /*
  22   │         Indent
  23   │         1       2       3       4
  24   │ 1       ?
  25   │ 22      ?
  26   │ 333     ?
  27   │ 4444    ?
  28   │ 55555   ?
  29   │ 666666  ?
  30   │ 7777777 ?
  31   │ 88888888        ?
  32   │ */

This changes how the files are named (to allow for snapshots that aren't directly related to the --style argument) and fixes the generate_snapshots.py script to work with the latest version of bat. Three new tests are also introduced: - tabs_4 - Tab expansion with a width of 4. - tabs_8 - Tab expansion with a width of 8. - tabs_passthrough - No tab expansion.

sharkdp · 2018-09-11T19:38:25Z

@eth-p Thank you very much for this contribution!!

My only concern is what I mentioned in #166:

One thing to keep in mind: Using a pre-processing step to expand the tabs could be problematic for languages with a syntax that distinguishes between tabs and spaces (i.e. Makefile).

To be honest, I didn't manage to construct an example that show-cases this issue because the Sublime text syntaxes seem to be rather forgiving, but in principle we could get syntax highlighting problems due to the replacement of tabs with spaces. As a stupid example, if we wanted to add a syntax for the Whitespace programming language, this approach would certainly be problematic 😄

Edit: I think I'm okay if we just blindly ignore this issue.

eth-p · 2018-09-11T20:22:41Z

@sharkdp Ah, I see what you mean now. That could be problematic.

It might incur a bit of a performance penalty, but I moved the tab expansion step to happen after the syntax is highlighted, so this should no longer be a problem.

Edit: There are a couple inconsistencies with tabs right now. I'm currently fixing them.

sharkdp · 2018-09-11T20:43:15Z

It might incur a bit of a performance penalty, but I moved the tab expansion step to happen after the syntax is highlighted, so this should no longer be a problem.

Fantastic! Let me know if this is ready for a full review.

# Conflicts: # src/printer.rs

eth-p · 2018-09-11T21:06:01Z

@sharkdp Everything's fixed. It should be ready for a review now.

sharkdp

This looks great, thank you very much for all your work!

Instead of adding more snapshot tests for the tab-expansion alone (which are hard to maintain), I think it would be great if we could use normal unit tests to make sure that the tab-expand-functionality works as expected. What do you think?

src/app.rs

sharkdp · 2018-09-11T21:41:56Z

src/app.rs

+                .matches
+                .value_of("tabs")
+                .and_then(|w| w.parse().ok())
+                .or_else(|| env::var("BAT_TABS").ok().and_then(|w| w.parse().ok()))


could the .and_then(|w| w.parse().ok()) part be used only once? Or are we dealing with different string types here?

It's implemented this way so that an invalid value for --tabs would fall back on the environment variable. Should it be done differently?

I think we should fail with an error message if the value for --tabs can not be parsed (instead of silently falling back on the environment-variable value).

Done in d404139

sharkdp · 2018-09-11T21:43:07Z

src/app.rs

@@ -41,6 +41,9 @@ pub struct Config<'a> {
    /// The character width of the terminal
    pub term_width: usize,

+    /// The width of tab characters.
+    pub tab_width: usize,


How is "no tab expansion" represented? tab_width==0? Could this be Option<usize> instead?

It could be, but it might complicate the argument parsing a little bit.

I would favor the Option version but I'm also fine with a short comment for now ("A value of zero indicates that no expansion should be performed").

Added a comment in d404139

sharkdp · 2018-09-11T21:46:30Z

tests/snapshots/sample.modified.rs

@@ -10,7 +10,7 @@ fn main() {
        "The perimeter of the rectangle is {} pixels.",
        perimeter(&rect1)
    );
-    println!(r#"This line contains invalid utf8:  "�"#;


What's going on in this changeset?

My IDE probably did something to that when I re-saved the file after adding the tabs section. I'll attempt to fix that.

eth-p · 2018-09-11T22:11:30Z

@sharkdp I could make a unit test for the tab expansion function itself, but I don't think it's worth it. The expansion function is pretty simple and straightforward, but the way it's used in the printer isn't. Changes to the printer are the most likely cause of it ever breaking in the future, and I don't think unit tests could cover that as well as the current snapshot tests.

sharkdp · 2018-09-12T16:48:32Z

@sharkdp I could make a unit test for the tab expansion function itself, but I don't think it's worth it. The expansion function is pretty simple and straightforward, but the way it's used in the printer isn't. Changes to the printer are the most likely cause of it ever breaking in the future, and I don't think unit tests could cover that as well as the current snapshot tests.

Ok 👍

sharkdp · 2018-09-12T17:00:53Z

tests/snapshots/sample.modified.rs

@@ -10,7 +10,7 @@ fn main() {
        "The perimeter of the rectangle is {} pixels.",
        perimeter(&rect1)
    );
-    println!(r#"This line contains invalid utf8:  "�"#;


There are still some changes in this line (which is why the snapshots also show modifications in this line).

The øˆ€€€ part is just the "interpolated" version of what comes out if you try to decode this as utf-8. You actually need to put the exact same bytes back in place (or tell me if you need any help).

I might need a bit of help with that, sorry haha...

Note that it can be pretty irritating when viewing the diffs in GitHub or some editors, as they will show the interpolated øˆ€€€ string in both cases (if you actually put the øˆ€€€ string in the file or if you put the invalid utf-8 sequence in the file)

Ok, I'll look into this later today.

Also added a comment to explain what 0 represents.

sharkdp · 2018-09-12T18:35:47Z

tests/tests.rs

+    grid_header_numbers:         "grid,header,numbers"         => [wrap: false, tabs: 8],
+    changes_grid_header_numbers: "changes,grid,header,numbers" => [wrap: false, tabs: 8],
+    full:                        "full"                        => [wrap: false, tabs: 8],
+    plain:                       "plain"                       => [wrap: false, tabs: 0],


@eth-p I had to change this to tabs: 0 because generate_snapshots.py produced a file that had tabs in them for --style=plain. I'm not sure why this was different for you. I tried to add --paging=never and --color=never to make this more robust.

@sharkdp That would be because it doesn't process tabs unless it needs to for alignment reasons (e.g. the pager or decorations are used). On second thought, though, that might be a bit confusing when people inconsistently end up with tabs sometimes.

Should I make a PR to remove that?

I think the current behavior is fine. I just wondered why I got a different version of the file than you when I ran the generate_.. script.

sharkdp · 2018-09-12T18:50:17Z

@eth-p Thank you very much for all of this!

eth-p added 4 commits September 10, 2018 18:11

Added tab expansion preprocessing step.

b23ff24

Updated snapshots.

7cdcdbb

Disabled tab expansion when decorations and pager are not used.

eb6e43b

eth-p mentioned this pull request Sep 11, 2018

Wrapping doesn't work properly with tabs. #166

Closed

Moved tab expansion to happen after syntax highlighting.

b4096e5

eth-p added 3 commits September 11, 2018 13:45

Fixed tab expansion not working in --wrap=never mode.

d90797f

Added three new snapshot tests for --tabs and --wrap.

c1e1f75

Ran cargo fmt.

1e74f0e

eth-p force-pushed the feature-tabs branch from baa1da0 to 1e74f0e Compare September 11, 2018 20:48

Merge remote-tracking branch 'upstream/master' into feature-tabs

1807f96

# Conflicts: # src/printer.rs

sharkdp requested changes Sep 11, 2018

View reviewed changes

eth-p added 2 commits September 11, 2018 15:03

Fix sample.modified.rs

503fe0b

Update snapshots.

9ef1e9f

sharkdp reviewed Sep 12, 2018

View reviewed changes

eth-p and others added 6 commits September 12, 2018 10:08

Added validation for --tabs.

d404139

Also added a comment to explain what 0 represents.

Undo change in sample.modified.rs

b7690d4

Re-generate files again

3e21d69

Make generate_snapshots script more robust

b4c6e41

Fix test for --style=plain

84ac92e

Make tests more robust

a05494e

sharkdp approved these changes Sep 12, 2018

View reviewed changes

sharkdp reviewed Sep 12, 2018

View reviewed changes

sharkdp merged commit 52d0d6c into sharkdp:master Sep 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tab expansion support. #302

Tab expansion support. #302

eth-p commented Sep 11, 2018 •

edited

Loading

sharkdp commented Sep 11, 2018 •

edited

Loading

eth-p commented Sep 11, 2018 •

edited

Loading

sharkdp commented Sep 11, 2018

eth-p commented Sep 11, 2018

sharkdp left a comment

sharkdp Sep 11, 2018

eth-p Sep 11, 2018

sharkdp Sep 12, 2018

eth-p Sep 12, 2018

sharkdp Sep 11, 2018

eth-p Sep 11, 2018 •

edited

Loading

sharkdp Sep 12, 2018

eth-p Sep 12, 2018

sharkdp Sep 11, 2018

eth-p Sep 11, 2018

eth-p commented Sep 11, 2018 •

edited

Loading

sharkdp commented Sep 12, 2018

sharkdp Sep 12, 2018

eth-p Sep 12, 2018

sharkdp Sep 12, 2018

sharkdp Sep 12, 2018

sharkdp Sep 12, 2018

eth-p Sep 12, 2018

sharkdp Sep 12, 2018 •

edited

Loading

sharkdp commented Sep 12, 2018

Tab expansion support. #302

Tab expansion support. #302

Conversation

eth-p commented Sep 11, 2018 • edited Loading

Implementation

Additional Information

Example

sharkdp commented Sep 11, 2018 • edited Loading

eth-p commented Sep 11, 2018 • edited Loading

sharkdp commented Sep 11, 2018

eth-p commented Sep 11, 2018

sharkdp 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

eth-p Sep 11, 2018 • 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

Choose a reason for hiding this comment

eth-p commented Sep 11, 2018 • edited Loading

sharkdp commented Sep 12, 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

sharkdp Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

sharkdp commented Sep 12, 2018

eth-p commented Sep 11, 2018 •

edited

Loading

sharkdp commented Sep 11, 2018 •

edited

Loading

eth-p commented Sep 11, 2018 •

edited

Loading

eth-p Sep 11, 2018 •

edited

Loading

eth-p commented Sep 11, 2018 •

edited

Loading

sharkdp Sep 12, 2018 •

edited

Loading