Skip to content

Added support for removing embedded ANSI color sequence for file appender#1001

Merged
nomiddlename merged 10 commits intolog4js-node:masterfrom
BlueCocoa:master
May 24, 2020
Merged

Added support for removing embedded ANSI color sequence for file appender#1001
nomiddlename merged 10 commits intolog4js-node:masterfrom
BlueCocoa:master

Conversation

@BlueCocoa
Copy link
Copy Markdown
Contributor

@BlueCocoa BlueCocoa commented May 1, 2020

It's quite common for developers to use embedded color sequence like \x1b[33m in the log string to make the output look beautiful.

However, if the output is also set to append to some file, the embedded color sequence will write to the given file as is, which may not be expected in some cases.

Thus I added one more option for file appender, which is removeColor. It's a boolean value that controls whether if the appender should detect and remove embedded color sequence before writing to file.

When this option is set to false, everything will be written to file as is. For example,

log4js.configure({
    appenders: {
        Core: { type: "file", filename: "app.log", removeColor: false },
        console: { type: "console" }
    },
    categories: {
        bot: { appenders: ["console", "Core"], level: "trace" },
        default: { appenders: ["console"], level: "trace" }
    }
})
const botLogger = log4js.getLogger("bot")
botLogger.debug("\x1b[33mHello\x1b[0m")

In the app.log, it will be [33mHello[0m, \x1b is invisible.

When this option is set to true, then embedded color sequence will be removed before writing to file.

log4js.configure({
    appenders: {
        Core: { type: "file", filename: "app.log", removeColor: true },
        console: { type: "console" }
    },
    categories: {
        bot: { appenders: ["console", "Core"], level: "trace" },
        default: { appenders: ["console"], level: "trace" }
    }
})
const botLogger = log4js.getLogger("bot")
botLogger.debug("\x1b[33mHello\x1b[0m")

Now in the app.log, it will be Hello only.

Signed-off-by: Cocoa [email protected]

@rnd-debug
Copy link
Copy Markdown
Contributor

rnd-debug commented May 2, 2020 •
edited
Loading

@BlueCocoa I am not a maintainer, but thank for the idea! Can you fix those ESLint errors?
image

There is a couple of other cases to consider, like three-digits color codes and codes separated by a semicolon (see https://en.wikipedia.org/wiki/ANSI_escape_code and https://superuser.com/questions/380772/removing-ansi-color-codes-from-text-stream)
Maybe something like this will be more accurate:
const regex = new RegExp("\x1b[[0-9;]*m", "g");

@BlueCocoa
Fixed ESLint errors
b18846c 
Signed-off-by: Cocoa <[email protected]>
@BlueCocoa
Copy link
Copy Markdown
Contributor Author

BlueCocoa commented May 2, 2020

@BlueCocoa I am not a maintainer, but thank for the idea! Can you fix those ESLint errors?
image

There is a couple of other cases to consider, like three-digits color codes and codes separated by a semicolon (see https://en.wikipedia.org/wiki/ANSI_escape_code and https://superuser.com/questions/380772/removing-ansi-color-codes-from-text-stream)
Maybe something like this will be more accurate:
const regex = new RegExp("\x1b[[0-9;]*m", "g");

Hi @rnd-debug, thanks for your kindly reply and the hint on ANSI color codes. I've made corresponding changes in the second commit. I think that should fix all the issues that ESLint complained.

@BlueCocoa
Copy link
Copy Markdown
Contributor Author

BlueCocoa commented May 2, 2020

According to the ESLint docs, perhaps we should turn off no-control-regex rule for this file?

@BlueCocoa BlueCocoa changed the title Added support for removing embedded color sequence for file appender Added support for removing embedded ANSI color sequence for file appender May 2, 2020
@codecov
Copy link
Copy Markdown

codecov Bot commented May 2, 2020 •
edited
Loading

Codecov Report

Merging #1001 into master will decrease coverage by 0.09%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
- Coverage   98.64%   98.55%   -0.10%     
==========================================
  Files          25       25              
  Lines        1030     1035       +5     
==========================================
+ Hits         1016     1020       +4     
- Misses         14       15       +1
Impacted Files Coverage Δ
lib/appenders/file.js 97.43% <80.00%> (-2.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5175f3f...52185a4. Read the comment docs.

nomiddlename
nomiddlename requested changes May 6, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nomiddlename nomiddlename left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, much appreciated. (Also thanks to @rnd-debug for their suggestions as well). I've made a couple of comments, see if you think they make sense.

Comment thread lib/appenders/file.js Outdated
Comment thread lib/appenders/file.js Outdated
@BlueCocoa
Copy link
Copy Markdown
Contributor Author

BlueCocoa commented May 7, 2020

Hi @nomiddlename, much thanks to your suggestions, corresponding changes has been made and pushed :-D

@nomiddlename
Copy link
Copy Markdown
Collaborator

nomiddlename commented May 7, 2020

Sorry @BlueCocoa - I just thought of a problem with this. The loggingEvent.data array can contain things that are not strings, for example if you do this:

const thing = { a: 'value' };
logger.debug('this is the thing: ', thing);

I think your code will throw an error in this case.

@nomiddlename nomiddlename added this to the 6.3.0 milestone May 24, 2020
@nomiddlename nomiddlename merged commit ff38084 into log4js-node:master May 24, 2020
@nomiddlename
Copy link
Copy Markdown
Collaborator

nomiddlename commented May 24, 2020

Fix published in [email protected]

@mkobayashime mkobayashime mentioned this pull request Nov 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nomiddlename nomiddlename nomiddlename approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@BlueCocoa @rnd-debug @nomiddlename