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

issue 2561, send all [bat error]s to stderr in default_error_handler #2827

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

Conversation

deboard
Copy link

@deboard deboard commented Jan 2, 2024

issue 2561, send all [bat error]s to stderr in default_error_handler match default.

@deboard
Copy link
Author

deboard commented Jan 3, 2024

So, here is my entry into the CHANGELOG.md:

The check is failing... Any ideas?

@keith-hall
Copy link
Collaborator

The CI logs show:

Grepping for PR info

I'm guessing it wants the PR # instead of the issue # in the changelog entry. So try with #2827 instead of #2561.

@deboard
Copy link
Author

deboard commented Jan 3, 2024 via email

@deboard
Copy link
Author

deboard commented Jan 3, 2024

This issue appears to be tied to if "attached_to_pager" is set to true in the Controller, stdout gets passed to default_error_handler (in error.rs) if so. A good fix may be to just always write "[bat error] {error}" to stderr in in the default section of the match in default_error_handler (error.rs@70).

I have tried this. It fixes the problem and doesn't doesn't mess with the pager functionality.

If you currently run bat like "bat --pager=never /tmp 2> /tmp/bat.txt", it handels redirection correctly.

exit 1
fi

exit 0
Copy link
Owner

Choose a reason for hiding this comment

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

Can we please turn this into a Rust-based integration test. See tests/integration_tests.rs and search for .stderr to see some examples.

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Owner

Choose a reason for hiding this comment

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

This script is still here. Can it be removed?

Comment on lines +1207 to +1208
#[test]
fn send_all_bat_error_to_stderr() {}
Copy link
Owner

Choose a reason for hiding this comment

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

What is this?

Comment on lines +7 to +10
- Send all bat error messages to stderr, see #2827 (@deboard)

- Fix long file name wrapping in header, see #2835 (@FilipRazek)
-
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the empty lines

@chicks-net
Copy link

This looks so close to being ready. Was there any particular reason this was abandoned?

@deboard
Copy link
Author

deboard commented Sep 26, 2024

I'll take a new look at it. I can't remember. Was I waiting for a review? Not sure. I'll get back at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants