Take `self` by value in build()

[jyn514]

Previously, build() would take &mut self and panic if called more than once. This is not ideal, especially since it wasn't documented anywhere except in the source code. This now enforces that build() is only called once by taking self in build.

Unfortunately, that meant that chains like the following no longer worked:

    let stylish_logger = Builder::new()
        .filter(None, LevelFilter::Error)
        .write_style(WriteStyle::Always)
        .build();

error[E0507]: cannot move out of a mutable reference
  --> examples/direct_logger.rs:27:26
   |
27 |       let stylish_logger = Builder::new()
   |  __________________________^
28 | |         .filter(None, LevelFilter::Error)
29 | |         .write_style(WriteStyle::Always)
   | |________________________________________^ move occurs because value has type `env_logger::Builder`, which does not implement the `Copy` trait

It would have to instead be rewritten like this:

    let stylish_builder = Builder::new();
    builder.filter(None, LevelFilter::Error).write_style(WriteStyle::Always);
    let stylish_logger = builder.build();

To avoid this, I changed all the builder functions taking &mut self -> &mut Self to instead take self -> Self. That fixes the example I gave, but broke a few others:

error[E0382]: borrow of moved value: `builder`
  --> src/filter/mod.rs:36:21
   |
15 |         let mut builder = Builder::new();
   |             ----------- move occurs because `builder` has type `env_logger::filter::Builder`, which does not implement the `Copy` trait
...
19 |            builder.parse(filter);
   |            ------- value moved here
...
23 |             filter: builder.build()
   |                     ^^^^^^^ value borrowed here after move

Those I just rewrote to use Builder::new().parse(filter).build() instead, since that seems more idiomatic.

This is a breaking change.

Alternatives

• Take self in build() and related constructors, and ask users to use builder.x(); builder.build() instead
• Don't change the API at all

[jplatte]

Another alternative would be to not invalidate the builder in .build(). This would require at least one public API change, as the format function is currently not required to be Clone. I think I prefer this solution though.

[jplatte]

@KodrAus wdyt?

[mainrs]

Isn't this still a breaking change, no matter what approach we take (this or yours @jplatte)? I do also prefer this solution though.

[jplatte]

Yes, would be a breaking change either way.

[KodrAus]

I personally prefer by-value builders so would be up for a new release that changed these to work that way.

I think originally we went with by-reference in the first place because you used to have to detect environment variables and yourself and pass the value through to the builder. Those cases where you conditionally interact with a builder are easier when they're by-reference, but since we have the Env API I don't think that's the case anymore.

We should be able to always configure env_logger in a single expression of chained builder methods.

[jplatte]

I've created a v0.9.x branch and merged this into that. I think this can wait for a bit, maybe some other breaking changes are going to come around.