-
Notifications
You must be signed in to change notification settings - Fork 191
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
Switch Tests to use the new logger #585
Switch Tests to use the new logger #585
Conversation
Updates runtests to use the ModelLogger
Codecov Report
@@ Coverage Diff @@
## master #585 +/- ##
==========================================
+ Coverage 72.17% 73.14% +0.97%
==========================================
Files 70 70
Lines 2016 2011 -5
==========================================
+ Hits 1455 1471 +16
+ Misses 561 540 -21
Continue to review full report at Codecov.
|
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.
This is great, thanks @arcavaliere! You somehow even increased test coverage lol.
I think you over-indented most of test_abstract_operations.jl
so would be good to fix that but otherwise looks good to merge.
Looking at the logs do you think it'll be more readable if we move the timestamp to the very beginning of the line? I guess since it's using 24-hour timestamps then it'll always be the same character width. Doesn't have to be part of this PR.
Also if you replace "#578" with "Resolves #578" in your original comment then merging this PR will also automatically close #578. A nice GitHub feature.
include("test_examples.jl") | ||
include("test_abstract_operations.jl") | ||
include("test_verification.jl") | ||
with_logger(ModelLogger()) do |
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.
This is pretty clean! I guess it makes sense to use with_logger
here but for log messages peppered around the rest of the package would global_logger(ModelLogger())
be better?
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 so! Do you think the line could be added in Oceananigans.jl
?
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.
Yeah we should do that, hopefully it doesn't also effect other packages. But we can leave it for another PR to avoid dragging this one out.
I think the timestamp coming first would be good. I also think having the LogLevel string be uppercase would also help readability. |
True, that would be nice! I'll merge this PR to avoid dragging it out but we should discuss how to improve the logger in an issue or future PR! |
Resolves #578
Updates runtests to use the ModelLogger
Updates tests to use @info instead of println