-
Notifications
You must be signed in to change notification settings - Fork 251
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
Fix setLogfile method to handle None path #813
Conversation
Could this not be accomplished with the following code?
|
Or even
If we want to keep the same syntax. But the PR looks good to me overall. It's just missing a test and an entry in the CHANGELOG @liangbug, and possibly one of these suggestions we made. |
I like the suggestion of @Joao-Dionisio ! |
8fc4de8
to
f13c198
Compare
I added unittest and changelog. @Joao-Dionisio, @Opt-Mucca, The follownng codes would cause compiling error.
Or you prefer the following code instead.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #813 +/- ##
==========================================
+ Coverage 51.85% 52.81% +0.96%
==========================================
Files 18 17 -1
Lines 3890 3834 -56
==========================================
+ Hits 2017 2025 +8
+ Misses 1873 1809 -64 ☔ View full report in Codecov by Sentry. |
@liangbug I'd prefer the code you suggested! |
@Opt-Mucca, I commited the code
|
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.
Thanks @liangbug for your contribution!
No description provided.