Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge --styles from different places using +/- #2929

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

eth-p
Copy link
Collaborator

@eth-p eth-p commented Apr 7, 2024

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?

The `overrides_with` clap builder option was removed
because it interfered with the matcher's ability to
retain all occurrences of `--style`.

The behavior it covered is expressed within the new
`forced_style_components` function.
Joining them with a space was causing certain styles (e.g. `-grid`) to
be misinterpreted as a separate option.
@eth-p
Copy link
Collaborator Author

eth-p commented Apr 7, 2024

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

@eth-p eth-p marked this pull request as draft April 7, 2024 01:19
#[test]
fn style_components_can_be_removed() {
bat()
.arg("--style=full,-grid")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.arg("--style=full,-grid")
.arg({
#[cfg(not(feature = "git"))]
{
"--style=full,-grid"
}
#[cfg(feature = "git")]
{
"--style=full,-changes,-grid"
}
})

This ensures the changes component will always get disabled.

.write_stdin("test")
.assert()
.success()
.stdout(" STDIN\n Size: -\n 1 test\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.stdout(" STDIN\n Size: -\n 1 test\n")
.stdout(" STDIN\n Size: -\n 1 test\n")

Less space is allocated to the sidebar when changes is disabled.

@einfachIrgendwer0815
Copy link
Contributor

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.

A huge thanks to @einfachIrgendwer0815 for helping me make sure
these tests work under the MSRV CI job.
@eth-p
Copy link
Collaborator Author

eth-p commented Apr 7, 2024

@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 eth-p marked this pull request as ready for review April 7, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants