Create builders for `Metadata`, `Record`, and `Level`.

[omh1280]

Fixes #116.

Create builders to support the construction of Metadata, Record, and Level. I decided to take mut self as the parameter for each builder method because it allows:

let record = RecordBuilder::new().args(...).metadata(...).build()

Whereas taking &mut self leads to the temporary object created by RecordBuilder::new() being dropped before build().

Not done yet--still need to add examples/docs. Please review everything else!

[brson]

Nit: can you add a blank line here?

[brson]

Example is missing closing backticks.

[brson]

@omh1280 thank you.

This all looks right to me with the exception that the builder pattern we tend to prefer is with &mut self instead of self. Unless there's a exceptional reason to use self here can you switch them to &mut self?

cc @alexcrichton can you peek at this? Do you agree that the builders should be by ref?

[brson]

Ditto about the blank line and missing backticks.

[brson]

Please add blank line here too.

[alexcrichton]

cc @alexcrichton can you peek at this? Do you agree that the builders should be by ref?

Ah yeah I agree that these should be &mut self across the board

[omh1280]

Gotcha. I'll update next time I get a chance!

[omh1280]

Fixed!

• I had to add Copy and Clone to prevent cannot move out of borrowed content in the build functions. Is this OK? Is there a better way to do this?
• As far as I can tell, RecordBuilder::new needs to take a reference to Location because it cannot create a default which has the appropriate lifetime.
• Finally, is there any reason Record contains a reference to Location instead of owning it?

Thanks for the review 😄

[brson]

Thanks @omh1280!

ping @alexcrichton

[brson]

Oh, I guess I can continue the review myself...

[brson]

I had to add Copy and Clone to prevent cannot move out of borrowed content in the build functions. Is this OK? Is there a better way to do this?

In this case, this seems fine. I don't see a reason for Record and Metadata not to be Copy/Clone. @alexcrichton what you think? An alternative would be for build to take self, but it looks to me like we prefer for builders to be reusable when possible.

As far as I can tell, RecordBuilder::new needs to take a reference to Location because it cannot create a default which has the appropriate lifetime.

Yes.

Finally, is there any reason Record contains a reference to Location instead of owning it?

I suspect this a subtle efficiency matter. This type is passed around during logging and copying one pointer is faster than copying the entire struct.

This patch looks fine to me now, but I'd be more confident if @alexcrichton took another look.

[alexcrichton]

I think we may want to remove Copy as it may be a bit restrictive, but adding Clone to these types seems fine by me!

Some other points I'd have:

• In addition to RecordBuilder::new perhaps we could also have something like Record::builder()? I think @sfackler has pioneered this pattern in the past and it can sometimes help the ergonomics of importing less items.
• Could you add to the documentation the default value for each builder method?

[omh1280]

Thanks Alex! Let me know if it still needs work (or if you want me to rebase to clean up commits)

[alexcrichton]

Thanks @omh1280! @sfackler I'm curious, what do you think about the 3 builders here vs trying to collapse them all into one? Or perhaps having one builder with methods that delegate to the others?

[sfackler]

These defaults are not going to be particularly useful - I'd probably lean towards making them empty strings.

[sfackler]

I can't actually remember why Location exists as a separate construct from Record I'd probably say we just inline those fields?

It does seem nice to have forwarding methods from the Record builder to the Metadata builder though.

[omh1280]

To confirm:

• Move __module_path, __file, and __line into Record and scrap Location? (and change macros etc.)
• Add forwarding methods so RecordBuilder can build entirely without MetadataBuilder?

[sfackler]

Yep!

If we're changing the macros, we may want to change the __log function to be publicly exposed and take a LogRecord directly, now that you can construct that with the public API.

[omh1280]

Sorry for the long wait--I'm in the middle of doing this I promise :)

[omh1280]

Updated!

This was a pretty big change, please let me know if I forgot something.

• Remove Location completely
• Inline module_path, line, and file into Record
• Forward target and level functions in RecordBuilder to MetadataBuilder

You now don't have to use any builder structs:

use super::{Record, Level};
let record = Record::builder()
    .module_path("foo")
    .file("bar")
    .line(30)
    .target("myApp") // forwarded
    .level(Level::Error) // forwarded
    .build();

I didn't make __log public: I can do that if that's agreed on /cc @alexcrichton

[alexcrichton]

This looks great to me! I'll leave @sfackler to sign off though.

@omh1280 it looks like there may be some CI failures as well?

[omh1280]

Didn't see that--fixed!

[sfackler]

Looks good to me but it needs a rebase.

[omh1280]

Squashed + rebased