Add Target::Pipe #195

TilCreator · 2021-02-25T15:42:06Z

Now logs can be written to custom pipes and not just to stdout or stderr.

This is my take on #178.

I would be very happy to hear some feedback, since I'm quite unsure about a few changes:

The example seams overly complicated, mostly because I didn't want an example to write to a file. Maybe there is a better candidate than std::sync::mpsc used as a pipe for that?
Builder::target_pipe is my idea for not breaking too much, but maybe there is a solution to implement that into Builder::target directly?
In Buffer it is assumed that if target_pipe is Some, test_target is always None. Is there a better way to enforce that?

TilCreator · 2021-02-25T20:23:51Z

I can't reproduce the failed check locally and I also didn't change anything on the code that is failing. Plz help?

jplatte · 2021-02-25T20:27:24Z

It's cargo clippy, not cargo check, that's failing (and with a nightly version of clippy). I've pushed a commit to master that fixes that lint plus one more, rebasing should get rid of the CI failure now.

TilCreator · 2021-02-25T20:39:02Z

Ah, I wasn't using the nightly version of clippy. I rebased master and the checks succeed now. thx

src/fmt/writer/mod.rs

jplatte · 2021-02-25T20:52:48Z

You did the rebase the wrong way around (you should rebase your branch on top of master, not the other way around).

Builder::target_pipe is my idea for not breaking too much, but maybe there is a solution to implement that into Builder::target directly?

What do you mean by "not breaking too much"? The derived traits for Target? I think I'd rather see those removed than there being a separate target_pipe builder method / field. I wonder why e.g. Hash is implemented for that in the first place.

jplatte · 2021-02-25T20:55:20Z

By the way, thank you for working on this!

TilCreator · 2021-02-25T21:02:43Z

Ups, I knew there was something wrong after at least the 5th git command, is it still worth trying to fix? ^^

jplatte · 2021-02-25T21:05:24Z

It should be as easy as

// if you haven't set up the remote yet, though I guess you have
git remote add upstream https://github.com/env-logger-rs/env_logger
git fetch upstream
// rebase your branch on top of upstream's master branch
git rebase upstream/master

even now. (git should detect the duplicate commit on your branch and get rid of it)

Followed by git push --force (--force to tell git you are sure you want to rewrite the history of the remote branch).

Now logs can be written to custom pipes and not just to stdout or stderr

Co-authored-by: Jonas Platte <[email protected]>

TilCreator · 2021-02-26T01:24:35Z

I have tried myself on keeping the pipe in the Target::Pipe, but that is just such a mess, so many Arc<Mutex<...>>....
Target also gets used a lot after build() to check if stdout or stdout is used, so removing or "circumenting" the derived traits of Target is hard. (The Builder is also not consumed on build().)

I will try something that takes the value out of Target::Pipe on target() or build() tomorrow.
Maybe I can even come up something better than enum Target {..., Pipe(Option<Box<dyn io::Write + Send + 'static>>)} after getting some sleep ^^

Forgot about it... again

TilCreator · 2021-03-02T21:19:28Z

I'm quite happy with this version as it breaks very little if anything at all and it feels like a simple extension of the old target method.
Now I just need a better example as the current one is just confusing and not super practical.

mainrs · 2021-03-07T22:20:39Z

I can take a closer look an Tuesday, up until then I don't have enough time for a full review :)

mainrs

The rest looks decent. Maybe @jplatte could look over again as he was the initial reviewer, and a second pair of eyes is always good ;)

That being said, sorry that it took me that long.

src/fmt/writer/termcolor/shim_impl.rs

mainrs · 2021-03-13T21:53:09Z

examples/custom_target.rs

+*/
+
+#[macro_use]
+extern crate log;


2018 idoms, you should be able to just call use log::{info, error, warn};. Is there a policy in-place for examples and what edition they should target? If not, I would use the newer syntax.

I would agree that this should be changed, but all other examples are very similar and use this line. So a bulk change of all the examples would be better. Should I still change it?

examples/custom_target.rs

src/lib.rs

Co-authored-by: SirWindfield <[email protected]>

TilCreator · 2021-03-13T22:09:42Z

thx @sirwindfield (also for fixing my typos ^^)

thx https://github.com/SirWindfield

jplatte · 2021-03-14T10:39:57Z

I can have a second look, sure. Probably going to take a while for me as well.

jose-acevedoflores · 2021-03-27T06:55:38Z

I've been playing around with this pr since I need support for logging to a file and this works great for my use case!

I did have one question about the possibility of having multiple targets like stdout and a file. Is this something out of scope for the goals of this library or is it something that could be entertained ?

TilCreator · 2021-04-06T17:51:28Z

(I'm just a contributor, not a maintainer)
I think this is possibly better implemented by the user, it sounds like a quite specific use case. (Maybe this could be added to the example (and I'm still not entirely happy with the example)?)

jplatte · 2021-04-06T17:57:07Z

src/fmt/writer/mod.rs

+    target_type: TargetType,
+    target_pipe: Option<Arc<Mutex<dyn io::Write + Send + 'static>>>,


Why split the target into two fields and introduce a new TargetType type? I think target can just be wrapped in Option for the build method to work (where you can then use self.target.take().unwrap_or_default() to get the actual value).

I did try that, because I would like it way more that way, too. But the Target gets passed (and cloned) everywhere (for color checking and the shim_impl) that is why TargetType needs Clone, Copy, Debug, Eq, Hash, PartialEq implemented which is hard to impossible with dyn io::Write + Send + 'static.

jplatte · 2021-04-06T17:59:40Z

src/fmt/writer/termcolor/extern_impl.rs

@@ -70,46 +71,72 @@ impl Formatter {

 pub(in crate::fmt::writer) struct BufferWriter {
    inner: termcolor::BufferWriter,
-    test_target: Option<Target>,
+    test_target_type: Option<TargetType>,
+    target_pipe: Option<Arc<Mutex<dyn io::Write + Send + 'static>>>,


Can you explain why Arc<Mutex<>> is needed?

Because the pipe is still partly owned by the Builder (it doesn't get deconstructed on build()).

src/fmt/writer/termcolor/shim_impl.rs

jplatte · 2021-04-28T22:01:46Z

I'm still planning to have a look at this again, hopefully this or next week.

jplatte · 2021-06-11T09:10:02Z

Merged in #204.

jplatte reviewed Feb 25, 2021

View reviewed changes

src/fmt/writer/mod.rs Outdated Show resolved Hide resolved

TilCreator and others added 4 commits February 25, 2021 22:07

Add Target::Pipe

28b8fca

Now logs can be written to custom pipes and not just to stdout or stderr

Fix example of target_pipe method

31033f4

Add Target::Pipe to the non termcolor BufferWrite

fc0b517

Fix typo

aebf1c2

Co-authored-by: Jonas Platte <[email protected]>

TilCreator force-pushed the custom_target_logging branch from 4e6919b to aebf1c2 Compare February 25, 2021 21:07

TilCreator added 2 commits March 2, 2021 21:59

remove target_pipe methode

fe00534

Fix the non termcolor BufferWrite

6cfa0d9

Forgot about it... again

TilCreator marked this pull request as ready for review March 4, 2021 10:56

mainrs suggested changes Mar 13, 2021

View reviewed changes

TilCreator and others added 3 commits March 13, 2021 23:05

Improve docs

92fd156

Co-authored-by: SirWindfield <[email protected]>

Fix typo

b6673ae

Co-authored-by: SirWindfield <[email protected]>

Fix typo

bda88f0

Co-authored-by: SirWindfield <[email protected]>

Simplify function

90f8a55

thx https://github.com/SirWindfield

jplatte reviewed Apr 6, 2021

View reviewed changes

src/fmt/writer/termcolor/shim_impl.rs Show resolved Hide resolved

Add nice newline

bac26d1

jplatte mentioned this pull request May 1, 2021

Add Target::Pipe cleaned up #204

Merged

jplatte closed this Jun 11, 2021

TilCreator mentioned this pull request Jun 14, 2021

Target::Pipe doesn't work #208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Target::Pipe #195

Add Target::Pipe #195

TilCreator commented Feb 25, 2021

TilCreator commented Feb 25, 2021

jplatte commented Feb 25, 2021

TilCreator commented Feb 25, 2021

jplatte commented Feb 25, 2021

jplatte commented Feb 25, 2021

TilCreator commented Feb 25, 2021

jplatte commented Feb 25, 2021 •

edited

Loading

TilCreator commented Feb 26, 2021 •

edited

Loading

TilCreator commented Mar 2, 2021

mainrs commented Mar 7, 2021

mainrs left a comment

mainrs Mar 13, 2021

TilCreator Mar 13, 2021

TilCreator commented Mar 13, 2021

jplatte commented Mar 14, 2021

jose-acevedoflores commented Mar 27, 2021

TilCreator commented Apr 6, 2021

jplatte Apr 6, 2021

TilCreator Apr 6, 2021

jplatte Apr 6, 2021

TilCreator Apr 6, 2021

jplatte commented Apr 28, 2021

jplatte commented Jun 11, 2021

		target_type: TargetType,
		target_pipe: Option<Arc<Mutex<dyn io::Write + Send + 'static>>>,

Add Target::Pipe #195

Add Target::Pipe #195

Conversation

TilCreator commented Feb 25, 2021

TilCreator commented Feb 25, 2021

jplatte commented Feb 25, 2021

TilCreator commented Feb 25, 2021

jplatte commented Feb 25, 2021

jplatte commented Feb 25, 2021

TilCreator commented Feb 25, 2021

jplatte commented Feb 25, 2021 • edited Loading

TilCreator commented Feb 26, 2021 • edited Loading

TilCreator commented Mar 2, 2021

mainrs commented Mar 7, 2021

mainrs left a comment

Choose a reason for hiding this comment

mainrs Mar 13, 2021

Choose a reason for hiding this comment

TilCreator Mar 13, 2021

Choose a reason for hiding this comment

TilCreator commented Mar 13, 2021

jplatte commented Mar 14, 2021

jose-acevedoflores commented Mar 27, 2021

TilCreator commented Apr 6, 2021

jplatte Apr 6, 2021

Choose a reason for hiding this comment

TilCreator Apr 6, 2021

Choose a reason for hiding this comment

jplatte Apr 6, 2021

Choose a reason for hiding this comment

TilCreator Apr 6, 2021

Choose a reason for hiding this comment

jplatte commented Apr 28, 2021

jplatte commented Jun 11, 2021

jplatte commented Feb 25, 2021 •

edited

Loading

TilCreator commented Feb 26, 2021 •

edited

Loading