-
Notifications
You must be signed in to change notification settings - Fork 617
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
[wpiutil] Refactor FileLogger #7150
Conversation
893e63a
to
d64ecc3
Compare
5d368ed
to
de864a9
Compare
c60af05
to
d605954
Compare
516ecb3
to
d409cb5
Compare
/format |
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.
Do we need to clarify the LineBuffer()
doc comment? Here's what it says for reference:
/**
* Creates a function that chunks incoming data into lines before calling the
* callback with the individual line.
*
* @param callback The callback that logs lines.
* @return The function.
*/
static std::function<void(std::string_view)> LineBuffer(
std::function<void(std::string_view)> callback);
I think the wording is ambiguous enough that it could be interpreted as either the old behavior or the new behavior, though I'm not sure if that's a reason for or against changing it.
Co-authored-by: Ryan Blue <[email protected]>
Co-authored-by: Ryan Blue <[email protected]>
5c4345d
to
cdbb8c3
Compare
/format |
cdbb8c3
to
415d8f8
Compare
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.
Overall, looks good! Maybe it's just me, but I'm still iffy about the doc comment wording- "Blocks of whole lines" could be interpreted as "blocks that are split at line endings" or as "blocks that each consist of a whole line". I think it's probably good enough for this PR to be merged- I don't think many people will look at Buffer()
anyways, especially since it's C++ (and by extension Python) only- but if anyone has any good ideas then maybe we could discuss those before merging this.
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 think this is good to merge for beta 1
Co-authored-by: Ryan Blue <[email protected]>
Co-authored-by: Ryan Blue <[email protected]>
Co-authored-by: Ryan Blue <[email protected]>
FileLogger now logs entire flushed blocks of data, instead of breaking it up into lines.
Fixes #7135.