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

Adding line number printing when output is piped out #2983

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

Conversation

domenicomastrangelo
Copy link

@domenicomastrangelo domenicomastrangelo commented Jun 1, 2024

Updating print_line function in SimplePrinter 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.

@@ -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

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

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

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.

3 participants