Make `--style` parameter more flexible #74

pitkley · 2018-05-06T19:00:15Z

The --style parameter now accepts a comma-separated list of strings, where every element defines either a single output component (changes, grid, header, numbers) or a predefined style (full, line-numbers, plain).

If available, bat picks the first predefined style in the user-supplied style-list and ignores everything else. If no predefined style was requested, the other parameters that are simple output components will be used.

Examples:

--style changes,full,numbers
Will be reduced to only the predefined style full.
--style plain,full
Will be reduced to only the predefined style plain.
--style changes,numbers
Will not be reduced, because the list does not contain any predefined styles.

(Note: if grid is requested but no other parameters, bat still creates the left-most column with a width of PANEL_WIDTH. I didn't want to introduce further logic in this PR that drops or adapts the width of the left column.)

^{(This PR supersedes #63.)}

sharkdp · 2018-05-07T18:13:38Z

src/main.rs

+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "full" => Ok(PredefinedStyle::Full),
+            "line-numbers" => Ok(PredefinedStyle::LineNumbers),


Just a small heads-up: Do we actually need this combined/predefined style if it is equivalent to "numbers"?

(there is no need to maintain backwards-compatibility. The --style option has not been released)

pitkley · 2018-05-07T20:07:29Z

I have updated the PR to work with the new Printer. (I have also decreased visibility modifiers where possible to not publish too much of the API we don't want public. I kept most of it on a separate commit, in case this is not wanted.)

From my tests this seems to work correctly, but I'd appreciate if somebody could verify this! 👍

BrainMaestro · 2018-05-07T20:17:39Z

src/printer.rs

+            self.print_horizontal_line('┼')
+        } else {
+            writeln!(self.handle, "File {}", self.colors.filename.paint(filename))
+                .map_err(Into::into)


Is this necessary? doesn't the ? operator suffice?

Since I am returning the result here, no. It would complain about getting a Result<_, io::Error> instead of Result<_, Error>. I can use ?; though and simply return Ok(()) at the end instead.

Let me know if you think that would be easier to understand!

BrainMaestro · 2018-05-07T20:23:44Z

src/printer.rs

+        );
+
+        // Print decorations
+        decorations


Hmm i'm not sure why this change was made. Is this better? There are more writes and a vector that isn't being used. Also, the changes don't seem related to the rest

I would assume that the write!s would be optimized extremely well, especially since we have stdout locked.

To me it felt wasteful allocating a Vec for every line, when we don't even have varying amount of elements in that vector. I'm fine with going back to a Vec on this, though.

After thinking about it a bit more, I have come up with the following solution:

let line_number_output = self.print_line_number(line_number).unwrap_or("".into()); let git_marker_output = self.print_git_marker(line_number).unwrap_or("".into()); let line_border_output = self.print_line_border().unwrap_or("".into()); let line = as_terminal_escaped( &regions, self.options.true_color, self.options.colored_output, ); write!( self.handle, "{}{}{}{}", line_number_output, git_marker_output, line_border_output, line ).map_err(Into::into)

What do you think about that?

BrainMaestro · 2018-05-07T20:36:30Z

src/main.rs

@@ -417,8 +479,7 @@ fn run() -> Result<()> {
        .arg(
            Arg::with_name("style")
                .long("style")
-                .possible_values(&["auto", "plain", "line-numbers", "full"])


no more list of possible values?

I have updated the handling of the comma-separated argument and could re-add the possible values! 👍

BrainMaestro · 2018-05-07T20:41:27Z

src/main.rs

+        match s {
+            "full" => Ok(PredefinedStyle::Full),
+            "plain" => Ok(PredefinedStyle::Plain),
+            _ => Err(ErrorKind::UnknownStyleName(s.to_owned()).into()),


Should it fail or just default to full? The other options take this more forgiving style

Defaulting to full happens in the clap-configuration: the default value for --style is full. Also, defaulting here would negate the selection-logic.

BrainMaestro · 2018-05-07T21:42:40Z

Hey @pitkley, seems good, but the parsing logic seems a little complicated. Something like this that uses a HashSet could be another way. Let me know what you think.

sharkdp · 2018-05-08T19:49:13Z

I'm sorry for not being too active on this pull request, but unfortunately this is quite hard to review as there are many changes that are unrelated to the actual feature. While some of these changes are really good, it would be better to split these into separate pull requests (in the future).

I agree with @BrainMaestro, the parsing logic seems quite complex. Maybe things would also be simpler if we remove plain and full completely and simply parse a list of components? The default value would simply be a list of all decorations. What do you think?

The `--style` parameter now accepts a comma-separated list of strings, where every element defines either a single output component (`changes`, `grid`, `header`, `numbers`) or a predefined style (`full`, `line-numbers`, `plain`). If available, bat picks the first predefined style in the user-supplied style-list and ignores everything else. If no predefined style was requested, the other parameters that are simple output components will be used. Examples: --style changes,full,numbers Will internally be reduced to only the predefined style `full`. --style plain,full Will internally be reduced to only the predefined style `plain`. --style changes,numbers Will not be reduced, because the list does not contain any predefined styles. (Note: if `grid` is requested but no other parameters, bat still creates the left-most column with a width of `PANEL_WIDTH`. I didn't want to introduce further logic in this PR that drops or adapts the width of the left column.)

This commit tries to simply the change. Instead of separating an `OutputComponent` and a `PredefinedStyle`, I have combined the two into just `OutputComponent`. To still have the styles work, I added an impl to `OutputComponent` with a function `components` which returns the components related to the specified component. For a simple output component this is trivial, but for the predefined styles this is a list of other components. The evaluating of the command-line parameter simplified significantly, making use of Claps `values_t!` macro to parse the supplied parameters into a `Vec<OutputComponent>`. After that it is simply a task of combining all supplied output components into one vector. Important: this means that the styles are now additive, rather than one of the predefined styles overruling other parameters supplied.

pitkley · 2018-05-09T19:16:45Z

I have rebased the PR and performed a few changes:

I have tried to change less of the surroundings. (The Cow change in printer has to stay though if we don't want to needlessly allocate strings required for grid-handling.)
I have pushed another commit on top. This one combines the two enums OutputComponent and PredefinedStyle into one and significantly reduces the parsing logic.

This is at the "expense" of the supplied styles now being additive, rather than one predefined style overruling. But this is probably more in line to what an end-user would expect.

(For more details, see the commit message.)

Let me know if this change fits better, or if there are more open points! 👍

Removing the plain and full styles altogether would simplify the logic even further, but we'd probably have to discuss how the plain-style would be achieved then. (Not sure if --style "" is ergonomic from an end-user perspective.)

BrainMaestro · 2018-05-09T19:36:59Z

src/main.rs

@@ -545,18 +598,17 @@ fn run() -> Result<()> {
                })
                .unwrap_or_else(|| vec![None]); // read from stdin (None) if no args are given

+            let output_components = values_t!(app_matches.values_of("style"), OutputComponent)?


very nice 👍

BrainMaestro · 2018-05-09T19:37:29Z

src/main.rs

+            let output_components = values_t!(app_matches.values_of("style"), OutputComponent)?
+                .into_iter()
+                .map(|style| style.components())
+                .fold(vec![], |mut acc, x| {


Vec::new() would be better since it's empty right?

vec![] is empty, too. I guess this is just preference, but results in the same code. (1, 2)

BrainMaestro · 2018-05-09T19:40:37Z

src/printer.rs

+            .output_components
+            .contains(&OutputComponent::Grid)
+        {
+            self.print_horizontal_line('┬')?;


Since this function is only relevant for grid, you could put a check in the function itself to print nothing if grid is not present. it would simplify a lot. what do you think?

That depends: the way it is implemented here right now, the header can also be shown regardless of if the grid is enabled or not. I suppose this is a decision to make: do we want to display a header if the grid is not requested and if so, how should it be formatted?

Sorry if I wasn't clear, I was actually specifically talking about print_horizontal_line, but you're right about the header thing. 🤷‍♂️

BrainMaestro · 2018-05-09T19:41:31Z

src/printer.rs

+
+            self.print_horizontal_line('┼')
+        } else {
+            writeln!(


This is not quite the same behavior. probably best to avoid the duplication

True, thanks for noticing. I'll leave this unchanged for the time being until we have a decision on whether header should have an effect without grid.

BrainMaestro · 2018-05-09T19:42:23Z

src/printer.rs

        write!(
            self.handle,
            "{}",
            decorations
                .into_iter()
-                .filter_map(|dec| dec)
+                .filter_map(|dec| if grid_requested {
+                    Some(dec.unwrap_or(" ".into()))


Hmm not sure I follow

In a grid we have to at least have a space for a non-existing element to have the right spacing in the left column. Unfortunately I haven't found a cleaner way to express this spacing, without going back to the multiple writes.

BrainMaestro · 2018-05-09T19:59:06Z

Nice, this is much easier to follow!

The Cow change, it probably has a negligible effect since we're still building strings after calling paint. Only 2-3 places where we have &str that can benefit while introducing many into calls.
As an extra suggestion, it would be nice if the checks could look like self.options.style.grid() and self.options.style.headers(). A simple tuple struct that wraps Vec<OutputComponent> would do this.
Also, wouldn't HashSet be better since there can be duplicates if the user passes --style=changes,full,header. Not that it affects anything

pitkley · 2018-05-09T20:10:39Z

The Cow change, it probably has a negligible effect since we're still building strings after calling paint. Only 2-3 places where we have &str that can benefit while introducing many into calls.

Agreed. I'll go back to Option<String>!

As an extra suggestion, it would be nice if the checks could look like self.options.style.grid() and self.options.style.headers(). A simple tuple struct that wraps Vec would do this.

I was thinking about how to make this simpler. I like your suggestion and will give it a try!

Also, wouldn't HashSet be better since there can be duplicates if the user passes --style=changes,full,header. Not that it affects anything

Good point, I'll update that too.

Thanks for the thorough reviews, by the way. I really appreciate them!

pitkley · 2018-05-09T20:40:58Z

Newest updates:

I have replace Cow by String. (It doesn't reduce as much noise since we need to convert &str to String, but it seems better anyway.)
I have replaced Vec by HashSet and wrapped the resulting HashSet<OutputComponent> into struct OutputComponents as suggested by @BrainMaestro. I kept the parameter name output_components to be in line with the struct, since renaming the struct to Style would have clashed with the Style from ansi_term. (If Style would be preferred though, I'd suggest going for something like use ansi_term::Style as AnsiStyle instead.)

BrainMaestro · 2018-05-10T02:22:24Z

src/printer.rs

-                Some(&LineChange::Modified) => self.colors.git_modified.paint("~"),
-                _ => Style::default().paint(" "),
-            }
+    fn print_git_marker<'s>(&self, line_number: usize) -> Option<String> {


unused lifetime

BrainMaestro · 2018-05-10T02:22:32Z

src/printer.rs

-    fn print_line_border(&self) -> Option<String> {
-        if let OptionsStyle::Plain = self.options.style {
-            return None;
+    fn print_line_border<'s>(&self) -> Option<String> {


BrainMaestro · 2018-05-10T02:23:24Z

Awesome! You've been very patient. Thanks for that

sharkdp

@pitkley I really like the new changes, thank you very much!

@BrainMaestro Thank you for reviewing!

pitkley mentioned this pull request May 6, 2018

Add style for line-changes only #63

Closed

pitkley force-pushed the flexible-style branch 3 times, most recently from 617ec34 to ee06c2e Compare May 6, 2018 19:16

sharkdp self-requested a review May 6, 2018 20:44

BrainMaestro mentioned this pull request May 6, 2018

Add printer #75

Merged

sharkdp reviewed May 7, 2018

View reviewed changes

pitkley force-pushed the flexible-style branch 2 times, most recently from f64bb5b to df6ce07 Compare May 7, 2018 20:04

BrainMaestro reviewed May 7, 2018

View reviewed changes

sharkdp added this to the 0.3 milestone May 8, 2018

sharkdp removed this from the 0.3 milestone May 8, 2018

pitkley added 2 commits May 9, 2018 20:56

pitkley force-pushed the flexible-style branch from a22af29 to 71684ea Compare May 9, 2018 19:12

BrainMaestro reviewed May 9, 2018

View reviewed changes

pitkley added 3 commits May 9, 2018 22:35

Create OutputComponents struct, use HashSet

dd98a89

Replace Cow by String

615ae17

Fix header-line if grid is not requested

86fa200

BrainMaestro reviewed May 10, 2018

View reviewed changes

Remove unused lifetimes

d429947

sharkdp approved these changes May 10, 2018

View reviewed changes

sharkdp merged commit ea27053 into sharkdp:master May 10, 2018

sharkdp mentioned this pull request May 10, 2018

A style like full but without line numbers or header #92

Closed

pitkley deleted the flexible-style branch November 1, 2020 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `--style` parameter more flexible #74

Make `--style` parameter more flexible #74

pitkley commented May 6, 2018 •

edited

Loading

sharkdp May 7, 2018

pitkley commented May 7, 2018 •

edited

Loading

BrainMaestro May 7, 2018

pitkley May 8, 2018

BrainMaestro May 7, 2018

pitkley May 8, 2018

pitkley May 8, 2018

BrainMaestro May 7, 2018

pitkley May 8, 2018

BrainMaestro May 7, 2018

pitkley May 8, 2018

BrainMaestro commented May 7, 2018

sharkdp commented May 8, 2018 •

edited

Loading

pitkley commented May 9, 2018 •

edited

Loading

BrainMaestro May 9, 2018

BrainMaestro May 9, 2018

pitkley May 9, 2018

BrainMaestro May 9, 2018

pitkley May 9, 2018

BrainMaestro May 10, 2018

BrainMaestro May 9, 2018

pitkley May 9, 2018

BrainMaestro May 9, 2018

pitkley May 9, 2018

BrainMaestro commented May 9, 2018

pitkley commented May 9, 2018

pitkley commented May 9, 2018

BrainMaestro May 10, 2018

BrainMaestro May 10, 2018

BrainMaestro commented May 10, 2018

sharkdp left a comment

Make --style parameter more flexible #74

Make --style parameter more flexible #74

Conversation

pitkley commented May 6, 2018 • edited Loading

Choose a reason for hiding this comment

pitkley commented May 7, 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

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

BrainMaestro commented May 7, 2018

sharkdp commented May 8, 2018 • edited Loading

pitkley commented May 9, 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

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

BrainMaestro commented May 9, 2018

pitkley commented May 9, 2018

pitkley commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrainMaestro commented May 10, 2018

sharkdp left a comment

Choose a reason for hiding this comment

Make `--style` parameter more flexible #74

Make `--style` parameter more flexible #74

pitkley commented May 6, 2018 •

edited

Loading

pitkley commented May 7, 2018 •

edited

Loading

sharkdp commented May 8, 2018 •

edited

Loading

pitkley commented May 9, 2018 •

edited

Loading