-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adding line number printing when output is piped out #2983
base: master
Are you sure you want to change the base?
Adding line number printing when output is piped out #2983
Conversation
tests/integration_tests.rs
Outdated
@@ -1779,7 +1779,7 @@ fn file_with_invalid_utf8_filename() { | |||
.arg(file_path.as_os_str()) | |||
.assert() | |||
.success() | |||
.stdout("dummy content\n"); | |||
.stdout(" 1 dummy content\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, is this really the expected behavior? The test invokes bat
without any line number style component arguments. I think it should stay as it was before - according to the issue being solved, line numbers should only be displayed when piping when the command line argument was explicitly passed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
I added a check for handle type here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't pass the test now, it seems like there may be an issue with the filename and the is_terminal() function or something like that
fn is_terminal(&self) -> bool
───────────────────────────────────────────
Returnstrue
if the descriptor/handle refers to a terminal/tty.On platforms where Rust does not know how to detect a terminal yet, this will return
false
. This will also returnfalse
if an unexpected error occurred, such as from
passing an invalid file descriptor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debugging it shows self.config.style_components.numbers() returns true, is there a different way to know if line numbers are requested that i'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess would be the following:
The SimplePrinter
is used whenever stdout is not interactive, --color
is not set to always
, --decorations
is not set to always
and --force-colorization
is not set either (see app.rs, config.rs and controller.rs). --style
controls which style components (such as header or line numbers) are used. The default for --style
is changes,grid,header-filename,numbers,snip
. This default currently stays the same even if loop_through == true
(which means that SimplePrinter
will be used). Currently, SimplePrinter
just does not care about --style
.
So, when loop_through == true
, the default for --style
should be plain
instead. Then your changes should work since they do when you specify --style=plain
explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the tip @einfachIrgendwer0815!
I updated the code and pushed the changes, now all the tests pass correctly and I removed all the integration test changes which were not necessary.
I also added a test for this specific case :)
…:domenicomastrangelo/bat into 2935-compatibility-issue-with-cat-piping
…:domenicomastrangelo/bat into 2935-compatibility-issue-with-cat-piping
Updating
print_line function inSimplePrinter to use line numbers when printing a line if line numbers are desired (self.config.style_components.numbers() / StyleComponent::LineNumbers).This was a compatibility issue with cat until now as describe in issue #2935.
Updating of some integration tests was necessary as now we expect the output to contain line numbers.