Repository navigation
Refactor config package and add functionality - #44
Conversation
7c2c2f1 to
80af8a5
Compare
80af8a5 to
9041602
Compare
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
This looks great! I love the surface simplicity, the separate yamlmap package, and the fact that extension authors are discouraged to interact with the core config directly. I have some polish-level comments
| hostsKey = "hosts" | ||
| ) | ||
|
|
||
| func TokenForHost(cfg *config.Config, host string) (string, string) { |
There was a problem hiding this comment.
Should these top-level functions take cfg as argument, or instead just access the config.Config instance internally as a singleton?
There was a problem hiding this comment.
Yeah I think I agree, these functions feel like they should know how to access the config themselves, I can't think of a scenario where we would want the user to pass in their own config.Config instance. My only worry is that config.Config becomes an implicit dependency of these functions rather than an explicit one.
| entries *yamlmap.Map | ||
| } | ||
|
|
||
| func Get(c *Config, keys []string) (string, error) { |
There was a problem hiding this comment.
Now that config.Config is just a struct and not some interface anymore, what are the tradeoffs of having config.Get/Set/Remove etc. be methods on the top level instead of on the struct itself?
There was a problem hiding this comment.
That is a good question. I don't know if I can think of a pro/con for either way. Perhaps having them be methods on the struct is more idiomatic? I don't have strong opinions on either direction.
There was a problem hiding this comment.
After thinking about this more I couldn't come up with a good reason not to make these methods so I made the change.
6e036a7 to
ae1157f
Compare
|
@mislav This is ready for re-review when you get a chance 🙇 |
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
This looks great. Thanks for your hard work and for the patience
2 participants
This PR introduces a bunch of functionality necessary for
ghto utilizego-ghfor config handling.Things changed:
configmaptoyamlmapconfigpackage with functions for manipulating configuration files. Only business logic that is encapsulated is around reading and writing of configuration files.authpackage with functions related to auth tokens and hosts. This functionality is not new, it just used to live in the internalconfigpackage.I added inline comments and questions for design decisions.
Note: This PR is not as long as it seems, lots of code was just shuffled around.
cc cli/cli#5560