search

Ogeon / rustful

[OUTDATED] A light HTTP framework for Rust
https://docs.rs/rustful
Apache License 2.0
862 stars 52 forks source link

Remove the logging system in favor of the Log crate #75

Closed kvikas closed 8 years ago

kvikas commented 8 years ago
  1. Removed log module.
  2. Added Log and env_logger as dev dependency.
  3. s/println!/log macros/ in example servers.
  4. Removed logging section from doc

Fixes #71

Ogeon commented 8 years ago

This looks good, so far. Replacing the println! lines in the examples with info! was a nice idea. Will they still print by default (I haven't yet tested, myself)? This looks much better than what we had before.

kvikas commented 8 years ago

If no environment is set it defaults to logging only error messages. Since the messages are logged with info!(), user has to run RUST_LOG=info ./target/debug/examples/tiny_hello to print all messages. Going by source of env_logger doesn't look like there is a way to override this behavior.

Ogeon commented 8 years ago

Hmm... Ok, those messages are supposed to be visible by default, so I think it's best to println! them, after all.

kvikas commented 8 years ago

Done and done.

Ogeon commented 8 years ago

Nice! It looks like that may be it. There are only a couple of formalities left:

I would like you to edit your PR description here to not have the "TODO" part, and change "Issue" to "Closes" or "Fixes", as I did for you in #73. The description goes into the final merge description, so it's nice if it's descriptive and doesn't contain uncompleted todo lists.

I would also appreciate it if you could rebase your commits so that the fixes are combined with the other two, or maybe just squash them together into one commit. It's easier to follow the changes if there are as few fixup commits as possible.

I will take another look at this tomorrow, when I'm not as tired, and see if I have missed something before I merge it. Thank you for doing this, it's really helpful.

Ogeon commented 8 years ago

I just realized how much Hyper prints in debug mode... That example is a little special, anyway, so I think this is good for now. Thank you!

@homu r+

homu commented 8 years ago

:pushpin: Commit fd75c77 has been approved by Ogeon

homu commented 8 years ago

:hourglass: Testing commit fd75c77 with merge 3763773...

homu commented 8 years ago

:sunny: Test successful - status