Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

paris: use a paris also for write_level #87

Closed
wants to merge 1 commit into from

Conversation

manio
Copy link
Contributor

@manio manio commented Nov 7, 2021

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:
image

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
@coveralls
Copy link

Coverage Status

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

@Drakulix
Copy link
Owner

Drakulix commented Nov 7, 2021

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
Copy link
Contributor Author

manio commented Nov 7, 2021

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
Copy link
Owner

Drakulix commented Nov 7, 2021

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

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

@manio
Copy link
Contributor Author

manio commented Nov 7, 2021

@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
Copy link
Contributor Author

manio commented Nov 8, 2021

@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: BurntSushi/termcolor#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
Copy link
Owner

Drakulix commented Nov 8, 2021

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 manio closed this Nov 9, 2021
@manio manio deleted the paris-level-colorize branch November 9, 2021 09:49
@manio
Copy link
Contributor Author

manio commented Nov 9, 2021

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

@manio
Copy link
Contributor Author

manio commented Nov 9, 2021

@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 #88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants