Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added core logging #5727
Added core logging #5727
Changes from 13 commits
f9fb5ab
163be76
8024720
ef68b27
b8950b6
ca15a98
b6441e6
baf6486
8924bb3
c670167
4d1e602
f1356c4
ba23794
ea1daa7
2ff0f8b
7d9d292
41f9d26
af0e164
0044a20
f63973b
50c64f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 it could also be useful to have tests for when we'd expect log output for
"warn"
and"error"
(and"fatal"
).I'm a little torn on whether we should split up some of the tests into dedicated
it
tests as well. If we add some more tests for"warn"
etc, having theit
separation may be favorable? 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 don't think we need separate tests, otherwise we are probably verifying that the whole logger works, even at the core level. We just need to be sure the callback is called, and get a confirmation that we can change the log level. I think we can assume that core verified that all the levels work.
Regarding "warn", "error" and so on, I was thinking about it, but it seems that those are actually raised only when using sync (like with a compensating write), and I thought it would have been too much to create a sync test just to verify the logger (also connected to what I've said before). I'm open to change my mind though 😁
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.
To me, the main reason for extending
"warn"
etc. would be if the tests are intended to test that the logger not only doesn't get fired when it shouldn't, but also that it does when it should. But if that's not within this scope then it might just be a PR for another time 👍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.
@RedBeard0531 I followed your suggestion of creating a
make_logger
method that is being used by the oldermake_logger_factory
method.I just wanted to ask if it makes sense the way that I've done it.
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'd like to iterate on this with you. I think it'd be possible to use the type parameters on the
fromEntries
,entries
andmap
to achieve this and avoidas unknown
.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 actually discovered that we don't need the
unknown
😁Before I didn't have the conversion to
number
, and that was the issueThere 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.
This suggestion is combined with my previous one if we provide the additional log level helpers.
In this case, you'd have to change e.g.
consoleErrorLevels.includes(logLevel)
toconsoleErrorLevels.includes(logLevel as ErrorLogLevel)
, so not sure how beneficial this would be for us internally.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.
Or perhaps
satisfies WarnLogLevel[]
.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.
@kraenhansen I wrote this as a comment to another related suggestion, but I'm reluctant to go this route (defining
WarnLogLevel
andErrorLogLevel
), because it will make the code more difficult to read, and we won't get much out of it.