Conversation
| pub(in crate::fmt) struct Buffer { | ||
| inner: termcolor::Buffer, | ||
| test_target: Option<Target>, | ||
| test_target: bool, |
There was a problem hiding this comment.
This change I'm not so sure about. I haven't really tried to understand the surrounding code, but it was the easiest fix.
I am confident that the behavior is still the same as before, but maybe there was some reason the test target was being stored before? Also, having the same field name as in BufferWriter but a different type seems like a code smell, but I couldn't really come up with something better due to lack of understanding of the code overall.
There was a problem hiding this comment.
At first I was a bit confused, but I can not find anything wrong and I think it's an improvement.
Maybe rename Buffer::test_target to Buffer::is_test?
There was a problem hiding this comment.
I'll check again how it's used and rename either to is_test or sth. like has_test_target.
There was a problem hiding this comment.
Ye, I do agree on the has_test_target.
|
@TilCreator please have a look. cc @sirwindfield, maybe you want to review the overall changes again? |
Now logs can be written to custom pipes and not just to stdout or stderr
Forgot about it... again
Co-authored-by: SirWindfield <SirWindfield@users.noreply.github.com>
jplatte
force-pushed
the
custom_target_logging
branch
from
483db73 to
5a8cf8e
Compare
|
Sure, I'll do it on Saturday! Sorry for the long delay, university has been eating up all my time. |
|
Doing it this evening, sorry for the delay :( |
mainrs
left a comment
mainrs
left a comment
There was a problem hiding this comment.
Looks good to me!
I am still confused on why clippy didn't pick up the len() > 0 statements and recommended is_empty() instead. Pretty sure there was a lint for that.
| pub(in crate::fmt) struct Buffer { | ||
| inner: termcolor::Buffer, | ||
| test_target: Option<Target>, | ||
| test_target: bool, |
There was a problem hiding this comment.
Ye, I do agree on the has_test_target.
3 participants
#195 with some adjustments to have the pipe as enum variant data instead of a separate field.