-
Notifications
You must be signed in to change notification settings - Fork 375
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
feat(gnoland): in config
, refer to fields using toml struct tags
#1769
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1769 +/- ##
==========================================
+ Coverage 47.49% 47.51% +0.02%
==========================================
Files 388 388
Lines 61305 61331 +26
==========================================
+ Hits 29117 29144 +27
+ Misses 29750 29747 -3
- Partials 2438 2440 +2 ☔ View full report in Codecov by Sentry. |
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.
Looks good to me. The smallest suggestion I have is to change the name of the callback function from cb
to something that describes what is going on; it also helps to understand what the boolean returned means.
At most I'd suggest removing the callback function in favor of an interface like
type valueSetter interface {
Set(name string, value reflect.Value) (ok bool)
}
And then make the necessary changes around it. I think that using interfaces over callbacks makes it easier to understand. Not required to be merged though.
done! as for your suggestion, it probably requires a bigger overhaul of the whole config system. keep in mind I designed it this way because it could also easily support just reading the fields (rather than setting them), so an "iterator-ish" approach is more proper IMO. |
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.
Looks good 💯
I've left minor comments 🙏
After briefly discussing #1605, me and @zivkovicmilos convened that it would be better if the
config
commands took, for the keys in the configuration, their TOML name rather than the internal Go struct field name. This PR implements that.