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 new --style called header-filesize and display it by default #1988

Merged
merged 15 commits into from
Feb 7, 2022

Conversation

mdibaiee
Copy link
Contributor

Fixes #1701

src/input.rs Outdated Show resolved Hide resolved
src/printer.rs Outdated Show resolved Hide resolved
src/printer.rs Outdated Show resolved Hide resolved
src/header.rs Outdated Show resolved Hide resolved
src/bin/bat/app.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@mdibaiee mdibaiee marked this pull request as ready for review December 29, 2021 16:36
@mdibaiee
Copy link
Contributor Author

I have not written tests yet, waiting for some feedback on my approach and implementation, I will make necessary changes and then write tests in the final steps

@mdibaiee mdibaiee force-pushed the 1701/header-info branch 2 times, most recently from 30a0639 to 7d7c461 Compare December 30, 2021 11:03
Cargo.toml Outdated Show resolved Hide resolved
@mdibaiee
Copy link
Contributor Author

mdibaiee commented Jan 3, 2022

Okay, so I moved the header fields into --style, and deprecated header value of --style in favour of all the others. If any of the header fields are enabled, StyleComponents.header() returns true, so it allows us to know whether we need to draw a header at all.

Example usage:

target/debug/bat --style grid,filename,filesize,permissions,lastmodified CONTRIBUTING.md

────────────────────────────────────────────────────────
File: CONTRIBUTING.md
Size: 1.6 KB
Permissions: 644
Last Modified At: 23 Dec 2021 21:16:17
────────────────────────────────────────────────────────

Note that I also added a deprecation warning for header value:

[bat warning]: Style 'header' is deprecated, use 'filename' instead.

This has broken a lot of tests, I will address those once I know we agree on the approach.

@mdibaiee mdibaiee force-pushed the 1701/header-info branch 2 times, most recently from 6c52de1 to d57bed2 Compare January 3, 2022 07:16
src/style.rs Outdated
@@ -11,6 +11,10 @@ pub enum StyleComponent {
Grid,
Rule,
Header,
Filename,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just so I don't forget: We should consider marking this enum as #[non_exhaustive], similar to #2012

@sharkdp
Copy link
Owner

sharkdp commented Jan 3, 2022

Note that I also added a deprecation warning for header value:
[bat warning]: Style 'header' is deprecated, use 'filename' instead.

I did not have time to review this at all, but dare to ask: would it maybe make sense to keep header around and change it's meaning to a default set of enabled header infos? Also, should we maybe prefix the other new options with header-? Something like:

header-filename
header-filesize
header-permissions
header-lastmodified
header = header-filename + header-filesize
header-full = header-filename + header-filesize + header-permissions + header-lastmodified

Just a suggestion - I haven't thought about it in detail.

@mdibaiee
Copy link
Contributor Author

mdibaiee commented Jan 3, 2022

@sharkdp I wouldn't mind that either, that might help by being:

  1. backward compatible (somewhat... since we might add more than just filename to the default)
  2. namespaces header components for clarity / ease of search

Let's hear what others have to say.

@Enselic
Copy link
Collaborator

Enselic commented Jan 3, 2022

I actually had header-filename first before I submitted the comment, but was afraid it was too verbose, so I simplified to filename. Since our initial instinct was the same, maybe we should go with header-filename etc after all. I am open to either way.

@mdibaiee
Copy link
Contributor Author

mdibaiee commented Jan 3, 2022

Okay, I will update the code according to @sharkdp's comment (hopefully tomorrow). Thanks!

src/bin/bat/clap_app.rs Outdated Show resolved Hide resolved
@Enselic
Copy link
Collaborator

Enselic commented Jan 9, 2022

I think this is starting to look good on a high level, so it's soon time to start detailed review.

One high-level thing to discuss is what header fields to show by default. Right now all new header fields are shown. I think Size is a reasonable default. Maybe even last modification time. But I'm not sure Permissions is worth showing by default. Especially since bat is read-only. What do you think of skipping showing Permissions by default?

I also think we should simplify Last Modified At to just Modified.

It would also be neat to align the header field values vertically. But I think it is fine to skip that in the first version of this feature.

One thing that might be worth doing in this initial PR is to also mark the other header fields values in bold, and not only the filename.

For context, here is current default output of cargo run examples/simple.rs in the PR.

───────┬──────────────────────────────────────────────────────────────────────────────────
       │ File: examples/simple.rs
       │ Size: 177 B
       │ Permissions: 644
       │ Last Modified At: 23 Jul 2021 18:57:19
───────┼──────────────────────────────────────────────────────────────────────────────────
   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 also took a look at what impact this PR has on the public API, using a new tool I wrote:

% cargo install cargo-public-items
% git checkout origin/master
% cargo public-items > /tmp/bat-master-api
% gh pr checkout 1988
% cargo public-items > /tmp/bat-header-info-api
% diff -u /tmp/bat-master-api /tmp/bat-header-info-api
--- /tmp/bat-master-api	2022-01-09 16:13:25.000000000 +0100
+++ /tmp/bat-header-info-api	2022-01-09 16:15:16.000000000 +0100
@@ -208,6 +208,11 @@
 bat::style::StyleComponent::Full
 bat::style::StyleComponent::Grid
 bat::style::StyleComponent::Header
+bat::style::StyleComponent::HeaderFilename
+bat::style::StyleComponent::HeaderFilesize
+bat::style::StyleComponent::HeaderFull
+bat::style::StyleComponent::HeaderLastModified
+bat::style::StyleComponent::HeaderPermissions
 bat::style::StyleComponent::LineNumbers
 bat::style::StyleComponent::Plain
 bat::style::StyleComponent::Rule
@@ -226,6 +231,10 @@
 bat::style::StyleComponents::fmt
 bat::style::StyleComponents::grid
 bat::style::StyleComponents::header
+bat::style::StyleComponents::header_filename
+bat::style::StyleComponents::header_filesize
+bat::style::StyleComponents::header_last_modified
+bat::style::StyleComponents::header_permissions
 bat::style::StyleComponents::new
 bat::style::StyleComponents::numbers
 bat::style::StyleComponents::plain

which looks like a reasonable diff.

@mdibaiee
Copy link
Contributor Author

mdibaiee commented Jan 9, 2022

@Enselic regarding the default, I prefer Filename and Filesize, leaving the rest to be specified by users if necessary (I rarely look up or use last modified date of files, but I can't say that for everyone).

I updated the code to make other component values bold as well.

So far then, I only need to write tests, but open to more feedback, too.

@Enselic Enselic changed the title Add --header-info option to show more information about file in header Show more information about file in header Jan 10, 2022
src/bin/bat/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.

Here comes another round of comments.

May I also ask you to keep the CI green from now on, and before the next round of review please? Let me know if you have troubles reproducing the failures in CI locally, and I'll help you out.

And thanks a lot for working on this PR!

src/bin/bat/clap_app.rs Outdated Show resolved Hide resolved
src/bin/bat/clap_app.rs Show resolved Hide resolved
src/bin/bat/clap_app.rs Outdated Show resolved Hide resolved
src/input.rs Outdated Show resolved Hide resolved
src/printer.rs Show resolved Hide resolved
src/printer.rs Show resolved Hide resolved
@mdibaiee mdibaiee force-pushed the 1701/header-info branch 2 times, most recently from df1376b to 3329a8f Compare January 17, 2022 21:49
@mdibaiee
Copy link
Contributor Author

Thanks @Enselic for the review. I went with Option 1, removed permissions and lastmodified, so now it's only filename and size. Let me know if your thoughts.

@Enselic
Copy link
Collaborator

Enselic commented Feb 5, 2022

I created a ticket where we can discuss this further. If we introduce a new style called default, we don't need to make header-filesize default yet. But we still have the option to make it default later. See #2061.

I hope we can take care of this in a way that does not cause annoying conflicts for you @mdibaiee. The easiest way to avoid conflicts is probably to create a PR that solves #2061 that is based on this PR. And when that PR is ready, we first merge this PR, and then directly after the other PR.

Since we want to have the bat master branch always releasable, it would be wise to wait a bit with merging this, just to play it safe.

@sharkdp
Copy link
Owner

sharkdp commented Feb 5, 2022

I would be in favor of this (implementing #2061 and keeping header-filesize out of default). But I'm also okay with being overruled, if everyone else thinks we should enable header-filesize by default.

@mdibaiee
Copy link
Contributor Author

mdibaiee commented Feb 6, 2022

Okay, so should I just wait then, or is there any action on my side you need?

@Enselic
Copy link
Collaborator

Enselic commented Feb 7, 2022

Since sharkdp has been clear on that he is OK with this being merged even though having some reservations, I think we should merge it actually. I think we have enough of a plan on how to remove header-filesize from the default style again if there is a backlash from users.

Personally I would have found it useful to have that info in the header by default. E.g. when looking around in rustdoc JSON files that can easily be 7 MB big, it takes several seconds to highlight the entire file. In cases like that, it is useful to be informed of that the file is huge. It makes bat/syntect seem less slow. And when it is not interesting to know the size of a file, it only takes up one line.

Another way to look at it is to see it as a proxy for the number of lines in a file, which can also occasionally be useful to know. When navigating in a new repo, it gives a quick indication of a file is interesting to explore further. We can't really print the actual number of lines in a file, since that would requires us to read the entire file up-front.

But I am also perfectly fine with if we change our mind and remove header-filesize by default. But I think we can try to have it as default.

Waiting with making it the default is for sure the most low-risk approach. But many of us write code for bat for fun, and it seems a lot more fun to me to merge this PR than to wait. And crucially, it is not prohibitively fun, but reasonably fun.

@mdibaiee I just realised we forgot to ask you to add a CHANGELOG.md entry for this change as per https://github.com/sharkdp/bat/blob/master/CONTRIBUTING.md. Could you do that please?

@mdibaiee mdibaiee force-pushed the 1701/header-info branch 2 times, most recently from f62f1b1 to 06d38bc Compare February 7, 2022 08:36
@mdibaiee
Copy link
Contributor Author

mdibaiee commented Feb 7, 2022

@Enselic that sounds more fun indeed, thanks!

@sharkdp
Copy link
Owner

sharkdp commented Feb 7, 2022

I think you accidentally committed changes to submodules in assets/syntaxes/02_Extra

@mdibaiee
Copy link
Contributor Author

mdibaiee commented Feb 7, 2022

@sharkdp fixed that, that always confuses me 😁

@Enselic Enselic changed the title Show more information about file in header Add new --style called header-filesize and display it by default Feb 7, 2022
@Enselic Enselic merged commit 312c8ef into sharkdp:master Feb 7, 2022
@mdibaiee mdibaiee deleted the 1701/header-info branch February 7, 2022 19:53
@Enselic
Copy link
Collaborator

Enselic commented Feb 7, 2022

Big thanks for your contribution @mdibaiee! And thank you for your patience with us :)

I think we can add more info in CHANGELOG.md, e.g. mentioning that header-filename is an alias to header now, but I couldn't figure out a good exact wording, so I say we can postpone that. We always clean up CHANGELOG.md right before a release anyway.

IMHO we need to fix #2061 before we can add timestamp and file permission info like you originally had in this PR. But I look forward to any future PR that you will create :)

Now I will try to give some attention to #1985 that has also been sitting around for a while (sorry about that)

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.

Extra file information in the header
4 participants