-
-
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
issue 2561, send all [bat error]s to stderr in default_error_handler #2827
base: master
Are you sure you want to change the base?
Conversation
So, here is my entry into the CHANGELOG.md:
The check is failing... Any ideas? |
The CI logs show:
I'm guessing it wants the PR # instead of the issue # in the changelog entry. So try with |
Thanks, that was it. Good to know.
…On Tue, Jan 2, 2024 at 11:52 PM Keith Hall ***@***.***> wrote:
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.
—
Reply to this email directly, view it on GitHub
<#2827 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJU7E6STSRDDSXQTI6WLCDYMTPZVAVCNFSM6AAAAABBKUMSBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZUHA2DMOJYG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Thanks,
John
321-356-2301
|
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 |
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.
Can we please turn this into a Rust-based integration test. See tests/integration_tests.rs
and search for .stderr
to see some examples.
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.
Will do.
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 script is still here. Can it be removed?
#[test] | ||
fn send_all_bat_error_to_stderr() {} |
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.
What is this?
- Send all bat error messages to stderr, see #2827 (@deboard) | ||
|
||
- Fix long file name wrapping in header, see #2835 (@FilipRazek) | ||
- |
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.
Please remove the empty lines
This looks so close to being ready. Was there any particular reason this was abandoned? |
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. |
issue 2561, send all [bat error]s to stderr in default_error_handler match default.