-
Notifications
You must be signed in to change notification settings - Fork 15
Ensure ConsoleLogger logs the correct log level of the message #106
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
Conversation
| return ''; | ||
| } | ||
| return params.map(param => this.stringify(param)).join(',\n'); | ||
| const extractedParams = params.length === 1 && Array.isArray(params[0]) ? params[0] : params; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the reasoning behind this change?
I don't see why its necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous logging was a bit unexpected, see for instance our log message:
// this.logger.info(`The GLSP server is ready to accept new client requests on port: ${currentPort}`);
The GLSP server is ready to accept new client requests on port: 5007 []
```ts
// this.logger.info('This is', 'just', 'a', ['test', 'message']);
This is [
"just",
"a",
[
"test",
"message"
]
]whereas with the fix it is more as I would expect it:
// this.logger.info(`The GLSP server is ready to accept new client requests on port: ${currentPort}`);
The GLSP server is ready to accept new client requests on port: 5007
// this.logger.info('This is', 'just', 'a', ['test', 'message']);
This is "just",
"a",
[
"test",
"message"
]What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get it. Looking at the current master, I get extactly the expected behavior you described:
3:09:06 PM info - [SocketServerLauncher] The GLSP server is ready to accept new client requests on port: 5007
3:09:06 PM info - [SocketServerLauncher] This is "just",
"a",
[
"test",
"message"
]
[GLSP-Server]:Startup completed. Accepting requests on port:5007The current implementation:
logAdditionals(...params: any[]): string {
if (!params || params.length === 0) {
return '';
}
return params.map(param => this.stringify(param)).join(',\n');
}also already as a guard check for empty/undefined arrays. So
The GLSP server is ready to accept new client requests on port: 5007 [] should never happen.
But what I'm more confused by is the special handling for arrays of index length 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably used the WinstonLogger which does all of that already correctly! In any case, the real culprit was the missing spread when forwarding the params from the individual message calls (log, info, debug, etc.). Now it should behave more as expected. Please have another look.
5acea56 to
c2ca817
Compare
c2ca817 to
fd56f98
Compare
What it does
How to test
Follow-ups
Changelog