bat icon indicating copy to clipboard operation
bat copied to clipboard

Merge `--style`s from different places using `+`/`-`

Open eth-p opened this issue 10 months ago • 5 comments

This commit aims to solve #1032, #1161, #1741, and #2894 by introducing a mechanism for merging the styles specified in the config file, BAT_STYLE environment variable, and the --style command-line argument.

This does so by treating each --style argument as a list of style components, and introducing a new + and - component prefix (e.g. +numbers, -grid) that allows --style lists to be merged together. When a style list consists only of prefixed components, it will be merged into the list that comes before it. If it contains any non-prefixed components, it will replace the list before it instead.

Ultimately, the style lists are merged into a final set of style components roughly following this pseudocode algorithm:

lists.fold(
    init=set(),
    |accumulator, list| {
        if list.components.has_any(|component| component.prefix is "") {
            accumulator.clear()
        }
        
        for component in list.components {
            if component.prefix == "-" {
                accumulator.remove_all(component.expand())
            } else {
                accumulator.add_all(component.expand())
            }
        }
        
    }
)

This preserves the existing behavior of --style taking precedence over BAT_STYLE or the config file, while also allowing users to add or remove styles from individual runs of bat.

Examples:

  • BAT_STYLE=full bat --style=plain => (none)
    Overriding the style from the config file or environment variable. (current behavior) image

  • bat --style=default,-header,-numbers => changes,grid,snip
    Removing components from the default/full style. image

  • BAT_STYLE=full bat --style=-grid => changes,header-filename,header-filesize,numbers,snip
    Removing components specified from the config file or environment variable. image

  • BAT_STYLE=grid,numbers bat --style=+header => grid,numbers,header-filename
    Adding components from the config file or environment variable. (current behavior) image

Questions & Answers

Q. Why introduce +component instead of using component without the prefix?
A. In part due to technical limitations, and in part to avoid breaking users' existing configurations by preserving the original behavior of --style.

Q. Why did you remove overrides_with?
A. It forced clap's argument matcher to discard all but the most recent occurrence of --style.

Unresolved Questions

  • ~~What should we do when there is only one --style, and it contains something like +numbers? It's currently treated like --style=numbers; would it be better to treat it like default,+numbers?~~

  • ~~Why is this test failing under the MSRV toolchain?~~

eth-p avatar Apr 07 '24 01:04 eth-p

Converting to a draft until I can figure out why one of the tests is failing when building with the MSRV toolchain.

eth-p avatar Apr 07 '24 01:04 eth-p

The test run on the MSRV toolchain runs cargo test without the git feature. Therefore, the style component full does not contain changes, so less space gets allocated to the sidebar. I commented two suggestions that should fix the error.

einfachIrgendwer0815 avatar Apr 07 '24 08:04 einfachIrgendwer0815

@einfachIrgendwer0815 Ah, that explains a lot! Thank you so much for pointing that out; it saved me a lot of debugging time. Rebased with your suggestion to fix the failing test :)

eth-p avatar Apr 07 '24 18:04 eth-p

When may we see this merged and released? This is extremely helpful.

ylluminate avatar May 26 '24 22:05 ylluminate

Rebased on top of https://github.com/sharkdp/bat/commit/b7e44c76dc7bfe1d4bf6693ac20abdbbc48754b9.

Also made a new commit to answer the question of how --style=+numbers/--style=-numbers would behave if --style was only specified once. It was more consistent and intuitive to apply the change to the "default" style, rather than applying it to an empty set of components.

eth-p avatar Jun 11 '24 02:06 eth-p