borntyping / rust-simple_logger

A rust logger that prints all messages with a readable output format.
https://crates.io/crates/simple_logger
MIT License
220 stars 48 forks source link

Fmt refactor #40

Closed cehteh closed 2 years ago

cehteh commented 2 years ago

refactors the formatting code, makes it more generic and future proof. tests passing for me, examples are working. but I haven't checked for performance regressions yet.

piegamesde commented 2 years ago

You still have the commits of the thread_ids feature, is this intended?

cehteh commented 2 years ago

You still have the commits of the thread_ids feature, is this intended?

Yes should be on top of it because it includes the formaters for those, in case 'thread_ids' doesn't become merged for some reason this can be be factored out.

cehteh commented 2 years ago

Back to draft, will fix the issues above tomorrow. Feedback still welcome.

borntyping commented 2 years ago

Lots of small comments but this definitely makes the formatting calls at the end a lot easier to read :)

cehteh commented 2 years ago

On 2021-09-09 03:32, Sam Clements wrote:

@.*** commented on this pull request.

         #[cfg(feature = "stderr")]  
  • eprintln!("{:<5} [{}] {}", level_string, target, record.args());

We've lost the {:<5} formatting for the level string with this change. I'm also not sure how spacing between the fields is done?

hence the 'draft' state. Will add the :<5 back later, spacing is done by the prefix/postfix. I am busy right now, will do that later.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/borntyping/rust-simple_logger/pull/40#pullrequestreview-750164757

borntyping commented 2 years ago

Sorry, didn't mean to rush you!

cehteh commented 2 years ago

got the width/alignment working, renamed the vars, imo UPPERCASE for the templates looking better. Still WIP.

cehteh commented 2 years ago

After #39 is merged this should be ready to be merged to, maybe some minor fixes. I intend to rebase this on #39 before making this final.

borntyping commented 2 years ago

Sorry, I can see a lot of work went into this and there's some clever code in here, but this adds a lot of complexity for very little benefit to a library I'll be maintaining for a very long time - I can't merge this. I think the original log() implementation could use some work but mostly to reduce the amount of code in there, whereas this adds a lot of new code that isn't needed.

cehteh commented 2 years ago

m'kay, i guess i go for fern in my projects then..

cehteh commented 2 years ago

m'kay, i guess i go for fern in my projects then..

Note, the thread_id and formatting refactoring was meant as base for what I planned intially, logging to a rust channel which then can have user implemented backends like writing to syslog or logfiles (which would be not part of this lib, except some example).

borntyping commented 2 years ago

fern is definitely what I'd recommend for uses like proper logfiles or sending to syslog. simple_logger will likely only every print to stdout/stderr.