[fix] Update level property to change transport level #1191

[ChrisAlderson]

This should fix #1191, see the added testcase.

[ChrisAlderson]

Also related #1167

[DABH]

@ChrisAlderson maybe I'm being dumb but if obj has level then how will it be undefined / do you actually enter this if? LGTM aside from that question :)

[ChrisAlderson]

The if-statement might be a bit redundant because logs with level = 'info' should never go in the log method since they are being filtered in winston-transport on L79. I added the if-statement because I followed the design of the other test cases in the test suite.

[DABH]

Right. Doesn't bother me much but maybe we could say assume(obj.level).equals('error') instead? Maybe some of the other test cases in the suite could also be edited if they have dead code. But anyway the fix works, that's the important thing :)

[ChrisAlderson]

It seems the if-statement is just another way to make the sure the test case fails if the implementation of the Logger is incorrect, so I wouldn't say it is dead code per se. You are right about assume(obj.level).equals('error'), which is already implemented, also makes the test fail. Guess we will see what @indexzero has to say.

Thanks for the review 👍

[DABH]

Fair point, you have my +1 :)

[DABH]

This discussion is superseded now by @ChrisAlderson 's refactor/tests branch

[indexzero]

Agree with what you said @ChrisAlderson:

the if-statement is just another way to make the sure the test case fails if the implementation of the Logger is incorrect, so I wouldn't say it is dead code per se.

As I was refactoring that code I briefly broke something and that assertion was very helpful 👍

[indexzero]

@ChrisAlderson this is definitely one approach, but I wanted to provide a code sample to discuss what the semantics for logger.level = VALUE should be.

const logger = winston.createLogger({
  level: 'info',
  transports: [
    new winston.transports.Console({ level: 'error' }),
    new winston.transports.File({ file })
  ]
});

logger.error('ZOMG bad things happened! Print them to the console and write them to the file!');
logger.info('Everything is fine, just write it to a file');

// Update logger.level
logger.level = 'silly';

From a semantics perspective do we think that silly level messages should start being written to both the File and Console transports? Or just the File transport since the Console transport already has its own explicit level set by the user?

There are two possibilities imho:

1. Log 'silly' to both: semantically this would mean that setting logger.level overwrites any explicitly set level on the individual transports.
2. Log 'silly' just to File: semantically this would mean that setting logger.level makes it the default level for the Logger and and transports that do not have an explicit level.

My gut tends towards the latter. If you agree then it's probably simpler just to not have TransportStream in winston-transport default to the Logger level (i.e. removing this line)

All of the tests are 💯with either approach and we should continue to flesh them out to make our semantics rock-solid.

[indexzero]

@ChrisAlderson @DABH opt'ed for the latter – the change wasn't exactly the way I predicted, but winstonjs/winston-transport#26 is close.