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

Add a --style=default option #2119

Merged
merged 9 commits into from
May 4, 2022
Merged

Add a --style=default option #2119

merged 9 commits into from
May 4, 2022

Conversation

IsaacHorvath
Copy link
Contributor

Hello, this is my first contribution, meant to resolve #2061. I added a new --style=default option that is effectively a carbon copy of --style=full, only I've taken out the file size in the header. I expect there to be more differences going forward, e.g. after implementing some of #1701.

I added a simple integration test that seems to pass okay, and updated the shell completions (though I only use bash so I haven't tested them).

I am brand spanking new to Rust, so apologies if I'm way out of line here. I'm hoping to try my hand at contributing a bit here and there to bat help practice.

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this! I took a look and couldn't find anything worth commenting on (except some nitpick in CHANGELOG). This looks great on first sight. Especially thanks for adding an integration test. Those are super great to have.

Before I set Approved I think I would like to find the time to play around a bit with the implementation too, but code-wise I see no problems so far. So I think I will be able to set this as Approved within max a week or so.

CHANGELOG.md Outdated Show resolved Hide resolved
IsaacHorvath and others added 2 commits March 11, 2022 15:35
Co-authored-by: Martin Nordholts <enselic@gmail.com>
@IsaacHorvath
Copy link
Contributor Author

Thanks for your feedback! I polished it up a bit, and I have a little more courage to snoop around the issues list now.

@Enselic
Copy link
Collaborator

Enselic commented Mar 26, 2022

Sorry for the delay, but I got some other things I need to take care of, and I got less spare time than I anticipated, so it might take some time still until I can take a second look.

@Enselic
Copy link
Collaborator

Enselic commented Mar 28, 2022

Shouldn't the file size be omitted by default with this PR? It does not seem to be the case when I try:

/Users/martin/src/bat
1166d6c Update help text for '--style'  2119  
% cargo run -- --no-config examples/simple.rs 
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s
     Running `target/debug/bat --no-config examples/simple.rs`
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: examples/simple.rs
       │ Size: 177 B
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ /// A simple program that prints its own source code using the bat library
   2   │ use bat::PrettyPrinter;
   3   │ 
   4   │ fn main() {
   5   │     PrettyPrinter::new().input_file(file!()).print().unwrap();
   6   │ }
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────

Again, big thanks for working on this, and sorry for not taking a second look sooner, but my last two weeks have been unusually busy.

@IsaacHorvath
Copy link
Contributor Author

Of course, I forgot to actually make it the default option 😅 Should be fixed now.

@Enselic Enselic added this to the v0.21.0 milestone Mar 29, 2022
src/bin/bat/clap_app.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

This now seems to work as it should:

% cargo run -- --no-config examples/simple.rs       
   Compiling bat v0.20.0 (/Users/martin/src/bat)
    Finished dev [unoptimized + debuginfo] target(s) in 3.45s
     Running `target/debug/bat --no-config examples/simple.rs`
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: examples/simple.rs
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ /// A simple program that prints its own source code using the bat library
   2   │ use bat::PrettyPrinter;
   3   │ 
   4   │ fn main() {
   5   │     PrettyPrinter::new().input_file(file!()).print().unwrap();
   6   │ }
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────
% cargo run -- --no-config examples/simple.rs --style=full
    Finished dev [unoptimized + debuginfo] target(s) in 0.30s
     Running `target/debug/bat --no-config examples/simple.rs --style=full`
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: examples/simple.rs
       │ Size: 177 B
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ /// A simple program that prints its own source code using the bat library
   2   │ use bat::PrettyPrinter;
   3   │ 
   4   │ fn main() {
   5   │     PrettyPrinter::new().input_file(file!()).print().unwrap();
   6   │ }
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────
% cargo run -- --no-config examples/simple.rs --style=numbers,grid
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s
     Running `target/debug/bat --no-config examples/simple.rs --style=numbers,grid`
─────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1 │ /// A simple program that prints its own source code using the bat library
   2 │ use bat::PrettyPrinter;
   3 │ 
   4 │ fn main() {
   5 │     PrettyPrinter::new().input_file(file!()).print().unwrap();
   6 │ }
─────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────

I left some additional comments that would be good to respond to, but apart from that I consider this Approved now. Good job and thanks again!

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Approved without reservation. Thank you for the updates.

@sharkdp
Copy link
Owner

sharkdp commented Apr 3, 2022

Thank you for your contribution. I think this part of help text needs updating:

 * auto: same as 'full', unless the output is piped.

I guess this is now: "same as 'default', unless …".

Also, it would be great if the man page (template) in assets/manual/bat.1.in could be updated accordingly.

@IsaacHorvath
Copy link
Contributor Author

I did already update the --style section of the manpage with:
Possible values: *default*, full, auto, plain, changes, header, header-filename, header-filesize, grid, rule, numbers, snip.
Is there something I missed?

I'll add the help text change now.

@Enselic
Copy link
Collaborator

Enselic commented May 4, 2022

Looks to me like @sharkdp's comments have been taken care of. Let's merge this now. Thanks again.

@Enselic Enselic merged commit adea895 into sharkdp:master May 4, 2022
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.

Introduce new --style called default that is not necessarily same as full
3 participants