search

guzru / winston-sentry

Sentry transport for winston logger for node js
MIT License
82 stars 37 forks source link

Fixes #30: Transport should not ignore configured log level #32

Closed shaharke closed 8 years ago

shaharke commented 8 years ago

Fixes #31: Sometimes I'm baffled by inheritance in JavaScript...

Before 0.1.2 the level was extracted from options and was set on this:

  // Set the level from your options
  this.level = options.level || 'info';

This was omitted in the latest release. I think that it's best to call the constructor of winston.Transport with the relevant options instead of setting those options on this. IMO it makes it clearer that those properties are intended for the parent class and not for the child.

@adambiggs WDYT?

shaharke commented 8 years ago

@adambiggs I'm merging this PR since this really is a big bug.

adambiggs commented 8 years ago

@shaharke sounds good. Sorry for introducing this bug!

Trying to understand how I broke it... I'm guessing this.level here wasn't the same variable as here? So after I changed it to this.options.level it was the same variable which caused the bug?

It looks like || this.options.level on line 62 isn't needed anymore then?

shaharke commented 8 years ago

@adambiggs you removed the this.level assignment in the constructor, which meant that the parent transport logic of filtering messages by level was not working.

And yes - || this.options.level on line 62 is redundant. I'll remove it.

adambiggs commented 8 years ago

Ah, that makes sense.