Codecov Report

❗ No coverage uploaded for pull request base (logging-changes-and-drop-old-debugger@be9f3ce). Click here to learn what that means.
The diff coverage is n/a.

@@                           Coverage Diff                            @@
##             logging-changes-and-drop-old-debugger   #11698   +/-   ##
========================================================================
  Coverage                                         ?   60.67%           
========================================================================
  Files                                            ?      629           
  Lines                                            ?    34040           
  Branches                                         ?     4798           
========================================================================
  Hits                                             ?    20655           
  Misses                                           ?    12375           
  Partials                                         ?     1010           

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 be9f3ce...0c9c301. Read the comment docs.

Eric Snow (@ericsnowcurrently) Having Off in LogLevel implies that we've to Off everywhere. For instance,

export function resolveLevelName(
    level: LogLevel,
    // Default to configLevels.
    levels?: winston.config.AbstractConfigSetLevels
): string | undefined {
    if (levels === undefined) {
        return getLevelName(level);
    } else if (levels === configLevels) {
        return getLevelName(level);
    } else if (levels === winston.config.npm.levels) {
        return npmLogLevelMap[level];
    } else {
        return undefined;
    }
}

Here level can be LogLevel.Off, so npmLogLevelMap should also have LogLevel.Off as key, which was the reason I had to put a dummy "off" everywhere.
A solution I can see is to remove Off from LogLevel and use undefined instead.

Eric Snow (@ericsnowcurrently) Having Off in LogLevel implies that we've to Off everywhere.
[snip]
Here level can be LogLevel.Off, so npmLogLevelMap should also have LogLevel.Off as key, which was the reason I had to put a dummy "off" everywhere.
A solution I can see is to remove Off from LogLevel and use undefined instead.

I'm fine with not adding LogLevel.Off. I think you're right. However, I think we need a different sentinel value to mean "disable logging". undefined implies "not set", which isn't right. As I suggested in my last review, better alternatives would be null, "off", or a dedicated value (like const DisableLogging = {};). What do you think?

(Also, it may make sense to have a separate setting to disable logging, rather than overloading "python.logging.level".)

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information