Create builders for Metadata, Record, and Level. #200

2017-06-19T23:10:31-05:00

ottaviohartman commented

2017-06-19 23:10:31 -05:00

(Migrated from github.com)

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!

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: ```rust 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 (Migrated from github.com) reviewed

2017-07-05 20:54:06 -05:00

src/lib.rs

					
				@ -632,0 +638,4 @@

				    /// The source file containing the message.

				    pub fn file(&self) -> &str {

				        self.file

Nit: can you add a blank line here?

brson (Migrated from github.com) reviewed

2017-07-05 20:54:24 -05:00

src/lib.rs

					
				@ -632,0 +638,4 @@

				    /// The source file containing the message.

				    pub fn file(&self) -> &str {

				        self.file

Example is missing closing backticks.

brson commented

2017-07-05 20:56:49 -05:00

(Migrated from github.com)

@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?

@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 (Migrated from github.com) reviewed

2017-07-05 20:57:17 -05:00

src/lib.rs

					
				@ -632,0 +759,4 @@

				    }

				    /// Invoke the builder and return a `Record`

				    pub fn build(&self) -> Record<'a> {

Ditto about the blank line and missing backticks.

brson (Migrated from github.com) reviewed

2017-07-05 20:57:36 -05:00

src/lib.rs

Please add blank line here too.

alexcrichton commented

2017-07-06 09:09:58 -05:00

(Migrated from github.com)

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

> 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

👍 1

ottaviohartman commented

2017-07-06 09:21:18 -05:00

(Migrated from github.com)

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

ottaviohartman commented

2017-07-07 09:44:16 -05:00

(Migrated from github.com)

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 😄

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`](https://github.com/omh1280/log/blob/4db1b30e671d3648f34d14cb8f15875bb6e9b52d/src/lib.rs#L678) _needs_ to take a reference to `Location` because it cannot create a default which has the appropriate lifetime. - Finally, is there any reason [`Record`](https://github.com/omh1280/log/blob/4db1b30e671d3648f34d14cb8f15875bb6e9b52d/src/lib.rs#L611) contains a reference to `Location` instead of owning it? Thanks for the review 😄

brson commented

2017-07-14 19:36:17 -05:00

(Migrated from github.com)

Thanks @omh1280!

ping @alexcrichton

Thanks @omh1280! ping @alexcrichton

brson commented

2017-07-14 19:39:17 -05:00

(Migrated from github.com)

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

brson commented

2017-07-14 19:46:46 -05:00

(Migrated from github.com)

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.

> 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 commented

2017-07-17 10:00:17 -05:00

(Migrated from github.com)

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?

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?

ottaviohartman commented

2017-07-20 12:39:06 -05:00

(Migrated from github.com)

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

alexcrichton commented

2017-07-21 16:32:10 -05:00

(Migrated from github.com)

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 (Migrated from github.com) reviewed

2017-07-21 21:17:34 -05:00

src/lib.rs

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

sfackler commented

2017-07-21 21:18:56 -05:00

(Migrated from github.com)

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.

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.

ottaviohartman commented

2017-07-24 14:40:49 -05:00

(Migrated from github.com)

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?

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 commented

2017-07-24 14:43:21 -05:00

(Migrated from github.com)

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.

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.

ottaviohartman commented

2017-08-01 14:10:46 -05:00

(Migrated from github.com)

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

ottaviohartman commented

2017-08-01 14:56:41 -05:00

(Migrated from github.com)

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

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: ```rust 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 commented

2017-08-09 09:32:39 -05:00

(Migrated from github.com)