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
Refactor config package and add functionality #44
Conversation
7c2c2f1
to
80af8a5
Compare
80af8a5
to
9041602
Compare
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.
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
pkg/auth/auth.go
Outdated
hostsKey = "hosts" | ||
) | ||
|
||
func TokenForHost(cfg *config.Config, host string) (string, string) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/config/config.go
Outdated
entries *yamlmap.Map | ||
} | ||
|
||
func Get(c *Config, keys []string) (string, error) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙇 |
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.
This looks great. Thanks for your hard work and for the patience
This PR introduces a bunch of functionality necessary for
gh
to utilizego-gh
for config handling.Things changed:
configmap
toyamlmap
config
package with functions for manipulating configuration files. Only business logic that is encapsulated is around reading and writing of configuration files.auth
package with functions related to auth tokens and hosts. This functionality is not new, it just used to live in the internalconfig
package.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