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

logger.isLevelEnabled('off') not functioning as expected #1362

Closed
thernstig opened this issue Jan 21, 2023 · 11 comments · Fixed by #1364, #1369 or #1375
Closed

logger.isLevelEnabled('off') not functioning as expected #1362

thernstig opened this issue Jan 21, 2023 · 11 comments · Fixed by #1364, #1369 or #1375
Labels
enhancement New feature or request
Milestone

Comments

@thernstig
Copy link

import log4js from 'log4js';
const logger = log4js.getLogger('test');
logger.level = 'error';
console.log(logger.isLevelEnabled('error'));
console.log(logger.isLevelEnabled('info'));
console.log(logger.isLevelEnabled('off'));

I would expect the last statement to print false but it prints true.

The end goal is to do something like this:

import log4js from 'log4js';
if (logger.isLevelEnabled('off')) {
  logger.level = 'error';
  // do something here
}
@lamweili
Copy link
Contributor

lamweili commented Jan 21, 2023

Based on my memory, OFF is the highest log level. So it would always return true.

log4js-node/lib/levels.js

Lines 96 to 104 in cfab25c

ALL: { value: Number.MIN_VALUE, colour: 'grey' },
TRACE: { value: 5000, colour: 'blue' },
DEBUG: { value: 10000, colour: 'cyan' },
INFO: { value: 20000, colour: 'green' },
WARN: { value: 30000, colour: 'yellow' },
ERROR: { value: 40000, colour: 'red' },
FATAL: { value: 50000, colour: 'magenta' },
MARK: { value: 9007199254740992, colour: 'grey' }, // 2^53
OFF: { value: Number.MAX_VALUE, colour: 'grey' },

@thernstig
Copy link
Author

thernstig commented Jan 21, 2023

@lamweili is there any way to achieve what I want? In most of the code we enable some global logging configuration, but in some pieces of code we need to check if log4js.configure() ever been called or initialized in any way. I could not find any existing API to do this.

@lamweili
Copy link
Contributor

I don't think there is any way to check at the moment. I think this can be a good feature to have. 🤗

Give me some time, should be easy to implement. Maybe it should return the existing configuration if configured.

@lamweili lamweili added the enhancement New feature or request label Jan 22, 2023
@lamweili lamweili added this to the 6.8.0 milestone Jan 22, 2023
@lamweili lamweili linked a pull request Feb 20, 2023 that will close this issue
@lamweili
Copy link
Contributor

@thernstig, would the new API log4js.isConfigured() returning a boolean be sufficient?

While I can possibly cache the passed-in configuration and return that, it would not reflect any programmatical changes as they don't go through the same flow. That would be outdated and not an accurate representation of the latest configuration.

@lamweili lamweili reopened this Feb 20, 2023
@lamweili
Copy link
Contributor

lamweili commented Feb 20, 2023

Merging the PR automatically closed this issue.
I re-opened this so that you can close it if you are satisfied with the new API.

The documentation for the new API is here at PR #1369.

configured - log4js.isConfigured()

isConfigured method call returns a boolean on whether log4js.configure() was successfully called previously. Implicit log4js.configure() call by log4js.getLogger() will also affect this value.

@thernstig
Copy link
Author

@lamweili that is great for my user case at least. Better than before since it did not exist before at all. Thanks 👍

@lamweili
Copy link
Contributor

Did it resolve your issue? You can close this if it does. Thanks!

@thernstig
Copy link
Author

@lamweili it works, thanks! I think the information you added about log4js.getLogger() implicitly calls log4js.configure() if not called before, should be mentioned in the API docs about Loggers - log4js.getLogger([category]). But that is an aside.

Another aside is that Docosaurus would have been nice to use :D

@lamweili
Copy link
Contributor

@thernstig, you are right, maybe that should be included in the Loggers - log4js.getLogger([category]) API docs.
Do you want to raise a PR so that it is attributed to you?

As for docusaurus, I'm not familiar, you can have the liberty of another PR! 😄

@lamweili
Copy link
Contributor

lamweili commented Mar 7, 2023

@thernstig, in log4js@6.9.0:

Do you want to close this issue? Or do you have some other needs?

@thernstig
Copy link
Author

thernstig commented Mar 7, 2023

@lamweili no other needs, I am very happy with your maintenance of the project! 😄

@lamweili lamweili closed this as completed Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants