Adding ability to configure colors for console by noyez · Pull Request #224 · estk/log4rs

noyez · 2021-07-10T00:28:39Z

(reopening from PR:#204, now includes tests and docs)

Specifically this useful for dark console backgrounds where the default
for 'Blue' for log level INFO is difficult to read.

Most of the changes are in src/encode/patter/mod.rs. I added default
parameters for deserializing. I added a function to describe the default
pattern string, and defined that at the top of the source file.
I added a color_map to the Encoder structs which is a HashMap that
defines the LogLovel->Color mapping. The color_map is of type
HashMap<Level, Option<Color>>, if the Option is None, then no styling
is applied.

Docs were added to src/encode/pattern/mod.rs to the function new_with_colormap()
Also, the test function was added fn check_color_hash() to src/encode/pattern/mod.rs which tests a single change to the default color-map then checks the resulting color-map to make that change was made and that remain default colors are unchanged.

TODO/Future: This patch only allows foreground color to be specified.
There are other aspects of styling like bold,italics,background color.
Does it make sense to allow for more detailed styling? That involves
more work and more work for the maintainer. I think foreground color will
probably cover most everyone's cases.

To use, the relevant config change is as follows:

  pattern: "%m"
  color_map:
    INFO: Blue
    TRACE: Black

Related: issue:#17, PR:#186, PR:#204
Continued from: PR:#194

* Rename to config_parsing * Add workflows for devel

)

* Reorganize config * allow missing docs so tests run * lint abatement * some renames

* Use anyhow/thiserror

Co-authored-by: shmapdy <richard.meester@gmail.com>

* WIP custom err handling * Compiling * Add an method to the public api * re-enable Debug

Co-authored-by: braindead <braindeaded@protonmail.com>

…stk#186) * pattern encoder: Set trace to default color, reset formatting after * Fix formatting of last commit * lol apparently rustfmt doesn't like commas? idk if this will work * Set trace color to cyan

Specifically this useful for dark console backgrounds where the default for 'Blue' for log level INFO is difficult to read. Most of the changes are in `src/encode/patter/mod.rs`. I added default parameters for deserializing. I added a function to describe the default pattern string, and defined that at the top of the source file. I added a `color_map` to the Encoder structs which is a HashMap that defines the LogLovel->Color mapping. The color_map is of type `HashMap<Level, Option<Color>>`, if the Option is None, then no styling is applied. TODO: Adding a feature means adding documentation, which isn't complete. I can do this and add to the PR, however before i continue i want to make sure this is the direction that the maintiner wants to take. TODO: Other testing may need to be done. I was testing with `cargo test --features file` with a modified config option in the yaml test config. As i am not familiar with this code base there may be other cases that need to be tested. TODO/Future: This patch only allows foreground color to be specified. There are other aspects of styling like bold,italics,background color. Does it make sense to allow for more detailed styling? That involves more work and more work for the maintiner. I think forground color will probably cover most everyone's cases.

noyez · 2022-04-30T14:12:16Z

Just merged code w/ v1.1.1.

Was there something missing on this patch that you'd like to see before it gets merged? I thought it was ready go to, but if want to see something more or something changed let me know and i'll accommodate.

estk · 2022-04-30T14:40:27Z

Thanks @noyez I'll review and let you know. Let's start by getting the CI passing.

src/encode/pattern/mod.rs

estk · 2022-04-30T14:44:51Z

src/encode/pattern/mod.rs

 #[derive(Derivative)]
 #[derivative(Debug)]
-#[derive(Clone, Eq, PartialEq, Hash)]
+#[derive(Clone, Eq, PartialEq)]


Why remove Hash derive?

Cargo was returning an error w/ Hash derived. So i removed it and it complied and cargo test fine (at least w/ rustc 1.60).

--> src/encode/pattern/mod.rs:657:5 | 652 | #[derive(Clone, Eq, PartialEq, Hash)] | ---- in this derive macro expansion ... 657 | color_map: HashMap<Level, Option<Color>>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Hash` is not implemented for `HashMap<Level, std::option::Option<Color>>` | = note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

I think we'll need to keep it derived for semver reasons

estk · 2022-04-30T14:47:00Z

src/encode/pattern/mod.rs

-                match record.level() {
-                    Level::Error => {
-                        w.set_style(Style::new().text(Color::Red).intense(true))?;
+                if let Some(Some(color)) = color_map.get(&record.level()) {


What happens when we get None? or Some(None)?

Then the text is not styled and output as regulate unstyled text.

estk · 2022-04-30T14:54:30Z

I left a few comments re. changes that will block us from merging for semver reasons. Also, I had a more general thought: I think that instead of using a HashMap<Level, Option<Color>>, we should probably define a

struct ColorMap {
 info: Color,
 ...
}

#[derive(SmartDefault,...)] 
enum Color {
 #[default]
 Black,
...
}

Then here we can write something structurally correct rather than just procedurally. Does that make sense @noyez ?

estk · 2022-04-30T14:55:34Z

src/encode/pattern/mod.rs

 /// The pattern encoder's configuration.
 #[cfg(feature = "config_parsing")]
-#[derive(Clone, Eq, PartialEq, Hash, Debug, Default, serde::Deserialize)]
+#[derive(Clone, Eq, PartialEq, Debug, Default, serde::Deserialize)]


Why remove Hash?

We need to keep this for semver reasons

estk · 2022-04-30T14:56:18Z

src/encode/pattern/mod.rs

+#[derive(Clone, Eq, PartialEq, Debug, Default, serde::Deserialize)]
 #[serde(deny_unknown_fields)]
 pub struct PatternEncoderConfig {
+    #[serde(default = "default_pattern")]


I dont think we can change the default rn due to semver reasons.

noyez · 2022-04-30T15:13:15Z

Does that make sense @noyez ?

Totally, i can work on this. not today but soon. This approach makes sense and i agree that its probably a better way to handle it instead of using hash map. it shouldn't take much time to make this change.

estk · 2022-04-30T15:14:06Z

@noyez thanks for that, and thanks for the work on this!

src/encode/mod.rs

…/noyez/log4rs into noyez_configurable_highlight_devel

noyez · 2022-05-05T21:07:18Z

Ok, now i believe its ready for your review. Pardon my simple usage of github's PRs. I created a new struct ColorMap, its very basic and simple. It has methods for set(level, color), get(level) and unset(level), the latter just sets the styling to None, which is no styling. I'm not sure this is the best interface, you may want to tweak it, just lmk and i will do it. I did not use SmartDefault since writing a default for the color map is pretty trivial, and SmartDefault wasn't an existing dependency. I tried to add some documentation, but i'm not the best with rustdoc. Its all relatively crude, but its also relatively simple so i didn't see the need to complicate it.

akashlama1998-icloud · 2022-10-01T15:20:36Z

What is the right syntax to add color? I tried finding it in documentation but wasn't able to 😔. Can someone please help me with that.

Dirreke · 2022-12-07T12:50:57Z

You can use {h({l})} to add color. You can check the documents @akashlama1998-icloud

dclause · 2023-11-06T17:20:17Z

src/config/raw.rs

      pattern: "%m"
+      color_map:
+        info: Blue
+        trace: Black


Even if this is a test, another color than black should be used here I guess, in order not to assume any terminal color-mode. No matter what color is chosen other than white or black.

dclause · 2023-11-06T17:21:07Z

src/encode/mod.rs

 }
+impl Default for Color {
+    fn default() -> Self {
+        Color::Black


Should it be "None" to match either black or white depending on shell color mode ?

dclause · 2023-11-06T17:21:52Z

src/encode/pattern/mod.rs

+/// A simple color map struct
+///
+/// You can use this struct to define a custom color map.
+/// This can be done using a serializer (i.e. config file) or pragmatically.


typo: programmatically

dclause · 2023-11-06T17:24:03Z

src/encode/pattern/mod.rs

+            trace: None,
+            debug: None,
+            info: Some(Color::Blue),
+            warn: Some(Color::Red),


Shouldn't this be orange ?

dclause · 2023-11-06T17:31:52Z

src/encode/pattern/mod.rs

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ColorMap {
+    // in future this represent other aspects of style, like bold/italics, etc.
+    trace: Option<Color>,


make those pub would allow:
let color_map = ColorMap { info: None, ... } directly

dclause · 2023-11-06T17:33:13Z

src/encode/pattern/mod.rs

+///! #           feature = "pattern_encoder"))]
+///! # fn f() {
+///! let mut log_color_map = ColorMap::default()
+///! log_color_map.info_color = Some(log4rs::encode::Color::Cyan);


Could not get that to work: info_color does not exists, and members seems private anyway here

dclause · 2023-11-06T17:37:35Z

src/encode/pattern/mod.rs

+    ///! # }
+    ///! # fn main() {}
+    ///! ```
+    pub fn new_with_colormap(pattern: &str, color_map: ColorMap) -> PatternEncoder {


Feels weird to combine new and colors here.
How about keep new, and add a with_color method ?
Something along this:
PatternEncoder::new(pattern).with_color(color_map)

dclause · 2023-11-06T17:39:15Z

src/encode/pattern/mod.rs

+#[cfg_attr(feature = "config_parsing", derive(serde::Deserialize))]
+#[cfg_attr(feature = "config_parsing", serde(deny_unknown_fields))]
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ColorMap {


I guess Option<Color> is ok, but that leaves us to only the few defined in the Color struct.
That would require to add a new entry in the struct Color for arbitrary string, that would be a color such as defined by colorful or other module for instance.

Creative-Difficulty · 2024-03-07T14:29:38Z

Progess @noyez @estk @akashlama1998-icloud @Dirreke @dclause ?

gadunga and others added 30 commits July 22, 2020 13:44

Expose errors as failure::Error (estk#138)

5c3c88d

Issue estk#129: Drop XML Config support (estk#137)

905183e

Rename file feature (estk#147)

f59f4e5

* Rename to config_parsing * Add workflows for devel

Update changelog

9c5b945

Add an init function which takes a RawConfig (estk#150)

0f4085e

Clippy

6c75bc0

Changelog, cleanup

9824445

json macro

461e4c6

qualify

fedf2df

typo

f6dfdc3

Remove unused dir from .gitignore

e6d5c6f

Init raw on all platforms

b190d48

Remove log4rs::FormatError::XmlFeatureFlagRequired (estk#156)

1e8114f

Expand env vars in the path for File and RollingFile appenders (estk#155

e9da948

)

Reorganize config (estk#157)

4b1b830

* Reorganize config * allow missing docs so tests run * lint abatement * some renames

Use anyhow/thiserror (estk#159)

5953720

* Use anyhow/thiserror

Change addtivity to additive to match the actual code (estk#163)

a674389

Errors 1.0 (estk#160)

b59a728

Co-authored-by: shmapdy <richard.meester@gmail.com>

clippy

ea3f2f3

Standard derives rebase (estk#175)

dfec821

Bump serde-value

9a02da9

Alpha 1 version

3e09e60

Update highlight colors to be the same as env_logger (estk#167)

467b85d

Custom err handler (estk#183)

c997e0e

* WIP custom err handling * Compiling * Add an method to the public api * re-enable Debug

bump ver

b25c9f0

fix: init_raw_config forcing max_log_level to Info (estk#200)

d6ddbb1

Co-authored-by: braindead <braindeaded@protonmail.com>

pattern encoder: Set trace to default color, reset formatting after (e…

36627ba

…stk#186) * pattern encoder: Set trace to default color, reset formatting after * Fix formatting of last commit * lol apparently rustfmt doesn't like commas? idk if this will work * Set trace color to cyan

Merge branch 'devel' into noyez_configurable_highlight_devel

59846d5

fixing rustfmt errors

b58d7b0

Merging with upstream v.1.1.1

d0d37df

estk reviewed Apr 30, 2022

View reviewed changes

src/encode/pattern/mod.rs Show resolved Hide resolved

estk reviewed Apr 30, 2022

View reviewed changes

src/encode/pattern/mod.rs Outdated Show resolved Hide resolved

estk reviewed Apr 30, 2022

View reviewed changes

Fixing for LINT test w/

884087d

estk reviewed Apr 30, 2022

View reviewed changes

noyez force-pushed the noyez_configurable_highlight_devel branch from 60eab70 to 8eb53f9 Compare May 5, 2022 13:21

Removing color_map as HashMap and adding ColorMap Struct.

225c5ee

noyez force-pushed the noyez_configurable_highlight_devel branch from 8eb53f9 to 225c5ee Compare May 5, 2022 14:59

estk reviewed May 5, 2022

View reviewed changes

src/encode/mod.rs Show resolved Hide resolved

estk marked this pull request as draft May 5, 2022 15:46

noyez added 5 commits May 5, 2022 16:38

Removing color_map as HashMap and adding ColorMap Struct.

82bb5e5

Merge branch 'noyez_configurable_highlight_devel' of ssh://github.com…

adb2564

…/noyez/log4rs into noyez_configurable_highlight_devel

Merge branch 'noyez_configurable_highlight_devel' of ssh://github.com…

2a974ce

…/noyez/log4rs into noyez_configurable_highlight_devel

Merge branch 'noyez_configurable_highlight_devel' of ssh://github.com…

a4f1cd0

…/noyez/log4rs into noyez_configurable_highlight_devel

Removing default_color_map in favor of Default trait.

5bd8586

noyez marked this pull request as ready for review May 5, 2022 21:06

noyez requested a review from estk June 30, 2022 13:39

dclause reviewed Nov 6, 2023

View reviewed changes

Uh oh!

Conversation

noyez commented Jul 10, 2021 • edited by estk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noyez commented Apr 30, 2022

Uh oh!

estk commented Apr 30, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estk commented Apr 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noyez commented Apr 30, 2022

Uh oh!

estk commented Apr 30, 2022

Uh oh!

Uh oh!

noyez commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akashlama1998-icloud commented Oct 1, 2022

Uh oh!

Dirreke commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Creative-Difficulty commented Mar 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

noyez commented Jul 10, 2021 •

edited by estk

Loading

noyez commented May 5, 2022 •

edited

Loading

Dirreke commented Dec 7, 2022 •

edited

Loading