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

normalize log level before setting #661

Merged
merged 3 commits into from
Mar 25, 2019
Merged

normalize log level before setting #661

merged 3 commits into from
Mar 25, 2019

Conversation

seanplwong
Copy link

Fix #660

@james-criscuolo
Copy link
Collaborator

Thank you for taking a look at this, it is very close to being mergeable. I have a small issue with this line: 8646cc6#diff-b45d4be906fc4401f1fd5133946b5250R178

We used to be able to set any string here, so if someone passed a custom connector with their own levels than I believe it would work. Now, if they were to pass that string, it would end up as undefined here.

The paths forward would either be:

  1. Make the type passed in Levels | number
  2. change the above line to set the string to what was passed if Levels[string] is undefined.

Choice 2 preserves behavior, so it should be the change made.

@seanplwong
Copy link
Author

seanplwong commented Mar 21, 2019

level used to be like this:

    level: {
      get: function() {return level; },
      set: function(value) {
        if (value >= 0 && value <=3) {
          level = value;
        } else if (value > 3) {
          level = 3;
        } else if (levels.hasOwnProperty(value)) {
          level = levels[value];
        } else {
          logger.error('invalid "level" parameter value: '+ JSON.stringify(value));
        }
      }
    },

So i think custom level is not handled previously as well, it will log an error.

And even after that, it will stubble upon LoggerFactory#genericLog()

  public genericLog(levelToLog: Levels, category: string, label: string | undefined, content: any): void {
    if (this.level >= levelToLog) {
      if (this.builtinEnabled) {
        this.print(console[Levels[levelToLog]], category, label, content);
      }

      if (this.connector) {
        this.connector(Levels[levelToLog], category, label, content);
      }
    }
  }

when this.level is string, it will return false no matter what and skip everything: 'anystring' < 1000 is false.
'1' < 3 will work tho.

@seanplwong
Copy link
Author

seanplwong commented Mar 21, 2019

but we could inverse the control here, just let connector decide what it wants to do.

Sean Wong added 2 commits March 21, 2019 12:09
Let connector decide what to log on its own.
Set connector earlier to reflect error to connector when setting level.
@egreenmachine egreenmachine merged commit 41cabdd into onsip:master Mar 25, 2019
@seanplwong seanplwong deleted the log-level branch March 26, 2019 06:15
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.

3 participants