paris: use a paris also for write_level

manio commented 2 years ago

Hi Victor, I've prepared another PR:

If we are using paris feature to have a colored terminal (and log),
then it is also nice to have a levels colored in the log as well.

This way one can easily view created log in a similar look (with colors)
as it is printed real-time on a terminal.

Tip: To view the log with colors using 'less' you can use:
less -R /path/to/log

And then I can easily scroll back/see my program log as in terminal and the log level colors are doing its job:

coveralls commented 2 years ago

Coverage decreased (-0.3%) to 71.429% when pulling 7c9d3a6d4bae1357abf23837725cf820ab27a554 on manio:paris-level-colorize into ec68e6ef9b03c9f9a45731338789d32910b25a1a on Drakulix:master.

Drakulix commented 2 years ago

I think colored log levels are in general a good idea, but I do not think that this feature should require paris, when termcolor should be sufficient.

So I would rather like to see an implementation for TermLogger, that does not depend on the paris feature being enabled.

manio commented 2 years ago

I see. Don't you mean an implementation for WriteLogger?

I have to see if methods like: term_lock.set_color(ColorSpec::new().set_fg(color))?; are also suitable for writelogger...

Drakulix commented 2 years ago

I see. Don't you mean an implementation for WriteLogger?

Yes of course, TermLogger does already have an implementation, right.

manio commented 2 years ago

@Drakulix Are you sure that it is possible to to do this with termcolor? I've changed the WriteLogger's W implementation trait from Write to termcolor::WriteColor and I've ended with: the trait termcolor::WriteColor is not implemented for std::fs::File Is it a good direction? Do you have any hints for me?

manio commented 2 years ago

@Drakulix I am still investigating and trying to solve your comment about using termcolor in a place of paris for WriteLogger. I found this thread interesting regarding this: https://github.com/BurntSushi/termcolor/issues/30 It is about the similar problem - coloring the text without actually writing it to terminal, eg std::io::Stderr. It seems that additional memory buffer is needed for this (termcolor::Ansi, termcolor::Buffer). As far as I understand the termcolor was created especially for printing to terminal/windows console in mind, even autor is admiting this:

[...] when I no longer want to support the old console APIs. I don't know when that day will come, but it's not today. And when it does come, I'll likely deprecate termcolor (or hand off maintenance to someone I trust) and then switch to something like the ansi_term crate.

I think that plain paris API is much simpler in its color formatter implementation for this purpose and as well as amount of source code added to get this goal...

My introduced function termcolor_to_paris should also cover the custom color configuration for specific log levels in simplelog config.

Drakulix commented 2 years ago

Alright, I have given this some more thought.

So it seems, you are right and termcolor is (sadly) not sufficient for this use-case.

The reason I did not want to depend on paris for this feature is, that this implementation is quite inefficient, because it needs to allocate a String for every log message, because that is the only api paris provides.

If users actively choose to use paris for their log messages, that is not really my (or rather simplelog's problem), which is why I was fine with the paris-macros in the first place. If you choose to use paris, you get worse performance in exchange for a fancy feature, which might be fine depending on the amount of logging you are doing.

But this feels a lot more implicit. I expect more users to want colored levels in their log files (although this is highly unix-dependent), then to straight up integrate with paris. And for them this trade-off is not very explicit, because that paris is used in the background to produce the colored levels is mostly an implementation detail. And for high-volume logging this will definitely make a difference and would be very much unexpected for a "simple logging library".

So this leaves us with two options:

Introduce a new optional dependency (urgh), like ansi_term.
Or use paris for this feature anyway (and maybe follow up with an ansi_term solution later...)

We cannot instead replace termcolor (e.g. with ansi_term), because then we loose windows support.

I am not really stoked to do either, but I will make this dependant on the amount of work, you are willing to put in:

I would prefer a solution with ansi_term, if you are up to do that
If you don't want to work on that, I will accept the paris-based solution, because it is better then nothing.
(or fix up paris to provide an api, that is more like the std::fmt api and does not need to do any allocation, but that is probably out of scope.)

In any way, this needs to have an additional config toggle. Something like write_log_enable_colors: bool either in the Config struct (which would mean this option is global) or in the constructor of the WriteLogger (so it can be customized on a per-logger basis). Not every user will be a fan of writing control sequences into a log file and it is certainly rather uncommon, so this toggle should default to false.

manio commented 2 years ago

Ah - stupid github - I renamed a branch and it closed my PR instead of leaving it open...

manio commented 2 years ago

@Drakulix Thank you for your lengthy explanation. According to your thoughts I choosed the version with ansi_term which you prefered. I created a new PR for this as github doesn't get my branch rename...

Superseded by https://github.com/Drakulix/simplelog.rs/pull/88

Drakulix / simplelog.rs

paris: use a paris also for write_level #87