-
Notifications
You must be signed in to change notification settings - Fork 40
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: Consider --logoutput
CLI flag properly
#645
Conversation
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
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.
LGTM!
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
Codecov Report
@@ Coverage Diff @@
## develop #645 +/- ##
========================================
Coverage 57.09% 57.09%
========================================
Files 122 122
Lines 14646 14646
========================================
Hits 8362 8362
Misses 5567 5567
Partials 717 717
|
Might be worth constructing these binding-paths using reflect (in a later PR), is best practice in C# where strings must match types (as it makes the dependency clear, and adds some compile time protection). I assume Golang has a func that can get the name of a property. |
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.
LGTM, left a suggestion for a future PR, cheers for the quick fix
Yes that's my thinking as well, doing so via struct tags, but I haven't found a nice way yet to do it.... |
A counterpoint is the comment John made today about WASM and avoiding reflection. TBD |
Relevant issue(s)
Resolves #644
Description
This is a one-line fix... https://github.com/sourcenetwork/defradb/compare/orpheus/fix/config-cli?expand=1#diff-f95036fc6ff63eb5fcf251353f47b191fa8a2d927d37eacbb51e47e74e5f71f4R108
Viper wasn't binding to the key because of a typo.
Otherwise the PR renames
Config.Logging
toConfig.Log
which I suggest is clearer. This way, for example, it's possible to configure loglevel likeDEFRA_LOG_LEVEL=debug
instead ofDEFRA_LOGGING_LEVEL=debug
.Tasks
How has this been tested?
CI
Specify the platform(s) on which this was tested: