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

Feature: Loglevel #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feature: Loglevel #159

wants to merge 1 commit into from

Conversation

zentus
Copy link

@zentus zentus commented Feb 8, 2023

I'm not sure if this is something you want in the repo, if not, you can just close the PR.

This PR will:

  • Replace all console.log, console.warn and console.error with the corresponding method of logger (in the library, not yet in the CLI)
  • Add an optional additional options argument as the first argument to init() (If the first argument is a function, it will still be interpreted as callback)
  • In options, a property loglevel can be set
  • Add a flag to the CLI --loglevel [number] that gets passed down to init() if defined

Log levels:

  • 0 = Log errors
    logger.error()
  • 1 = Log errors and non-debug messages
    logger.error(), logger.log()
  • 2 = Log errors, non-debug messages and warnings
    logger.error(), logger.log(), logger.warn()
  • 3 = Log errors, non-debug messages, warnings and debug messages (default)
    logger.error(), logger.log(), logger.warn(), logger.debug()

The default loglevel will log everything, as it was previously. I tried to the best of my ability to use the most suitable logger method for each existing message, but there could be some messages that you may want to use another method for, I'll leave it for you to decide.

Examples:

# CLI
espruino --loglevel 0
// If you wish, you can test it out in the package of my fork
const espruino = require("@zentus/espruino");
const options = {
  logLevel: 0
};
espruino.init(options, () => console.log('init callback'));

// This should still work as well
// espruino.init(() => console.log('init callback'));

@gfwilliams
Copy link
Member

Thanks! This looks like a good idea, and it's great it's not using another library for the logging.

My concern is that the Espruino IDE (espruino.com/ide / https://github.com/espruino/EspruinoWebIDE) uses this too, but I think that merging this would break that since it doesn't use index.js (that's just for the NPM library).

I think it would also break espruinotools (https://github.com/espruino/EspruinoAppLoaderCore/blob/893c2dbbe5a93fbb80d035a695663b4f4cca8875/lib/espruinotools.js) built from EspruinoTools with https://github.com/espruino/EspruinoWebIDE/blob/master/extras/create_espruinotools_js.sh

Would you be able to move the logger declaration into espruino.js? And then maybe try and give the two things above a quick check?

@zentus
Copy link
Author

zentus commented Feb 9, 2023

Ah, I see! Sure, I'll see what I can do.

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.

2 participants