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.

Sign up for GitHub

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

Jump to bottom

Adding line number printing when output is piped out #2983

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

Adding line number printing when output is piped out #2983

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bb411ab
Adding line number printing when output is piped out
May 31, 2024
debf7c3
Added CHANGELOG entry
Jun 1, 2024
a3234bf
cargo fmt --check
Jun 1, 2024
5727ccb
Fixed file_with_invalid_utf8_filename integration test
Jun 1, 2024
de19b52
Merge branch 'master' into 2935-compatibility-issue-with-cat-piping
domenicomastrangelo Jun 1, 2024
1eec98b
Add check for non-terminal/tty handle
Jun 1, 2024
9523e40
Merge branch '2935-compatibility-issue-with-cat-piping' of github.com…
Jun 1, 2024
76832ef
cargo fmt
Jun 1, 2024
8ad55ab
Trigger Build
Jun 1, 2024
320f42b
Added current line number prefix
Jun 2, 2024
f96427f
Added current line number prefix
Jun 1, 2024
c0c1fd6
Merge branch '2934-compatibility-issue-with-cat-piping' of github.com…
Jun 2, 2024
2ba6813
Added switch to plain style as default:w in case of loop_through = true
Jun 6, 2024
1f4d6f0
Changed default to be Plain in case of loop_through
Jun 6, 2024
b13049a
Merge branch 'master' into 2935-compatibility-issue-with-cat-piping
domenicomastrangelo Nov 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Collaborator

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?

Copy link
Author

@domenicomastrangelo domenicomastrangelo Jun 1, 2024 •
edited
Loading

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

Copy link
Author

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

───────────────────────────────────────────
Returns true 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 return false if an unexpected error occurred, such as from
passing an invalid file descriptor.

Copy link
Author

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?

Copy link
Contributor

@einfachIrgendwer0815 einfachIrgendwer0815 Jun 3, 2024 •
edited
Loading

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.

Copy link
Author

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 :)

}

#[test]
Expand Down