-
Notifications
You must be signed in to change notification settings - Fork 42
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 circular dep and extra permission error in logs #436
Conversation
Codecov Report
@@ Coverage Diff @@
## master #436 +/- ##
==========================================
- Coverage 91.60% 91.25% -0.35%
==========================================
Files 60 60
Lines 2800 2814 +14
==========================================
+ Hits 2565 2568 +3
- Misses 235 246 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
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.
Great trick, thanks @ml-evs!
If this works with the tests, then there is indeed no need to have the extra validator for log_level
.
A minor comment, since I am confused about this.
My only worry is that |
You should be able to see this if you start the server via |
I just checked this. And it seems that the logger is indeed not initialized. We could instead completely remove the log statements from |
Just tweaked this some more with (hopefully) improved error handling. Only thing I can't get is for a RuntimeError to crash the server if the config is borked. |
To summarise new behaviour (might not be what we actually want):
|
Yeah, I think as with the logging to files, this should just produce a very visible warning (through logging) and then use the default values.
Yir. Wait, was the first point the one we now cannot do due to the circular import? |
- Raise RuntimeError of a specific config file was requested, but not found - Emit warning if the config file was not found in the default location without a custom location provided
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
It's a little bit verbose now, but I think this is a sensible set up. Config emits warnings that are caught (and then reset) by the server and written to the main log. Any config load failures only warn and don't cause a crash, but clearly print that defaults are being used instead. I also moved all the |
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.
Quite verbose now, yes. But in this case, I think it really makes sense.
This is great work @ml-evs ! Chanks.
I have approved with a single comment. I may have missed something, so it might indeed be important to keep that line in. If not, just ping me and I'll quickly re-approve 👍
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Closes #435.
Reworks error handling of config and logging set up.