Skip to content
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

Feature/36 42 config improvements #44

Merged
merged 4 commits into from Mar 8, 2018

Conversation

andscoop
Copy link
Contributor

@andscoop andscoop commented Mar 6, 2018

@andscoop andscoop requested a review from schnie March 6, 2018 15:08
Copy link
Member

@schnie schnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PreRun stuff all looks great, just have some questions about the config structure / patterns.

config/config.go Outdated
@@ -52,8 +52,8 @@ func InitConfig() {
initProject()
}

func initCfg(path string, gettable bool, settable bool) cfg {
cfg := cfg{path, gettable, settable}
func initCfg(path string, gettable bool, settable bool, setD bool, d string) cfg {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andscoop this method signature is starting to feel a little off. Same for the init block of callers above it.

What if we added a NewCfg method to meta.go that acts as a "constructor" and returns a new cfg object, with default values. Seems like a lot of this the invocations of this method use the same defaults. Maybe we could cut out some boilerplate, add some default values, and let callers of NewCfg directly override specific values by name. Something like

PostgresUser := NewCfg(cfg{Name: "postgres.user", Default: "postgres"})

Really comes down to moving the initCfg function to live with the cfg type definition and using the NewXXX pattern. Rather than having a seemingly arbitrary list of parameters to pass every time, we can set default values, that are overridden specifically by name, rather than just appearing in a list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should rename meta.go to types.go as well. Then we could have multiple Cfg types that maybe share some group of defaults. For example, GettableCfg, SetableCfg. Not sure if multiple types is really needed yet.

Also, do we need a setD param here? Could we get away with just checking if the Cfg has a Default value set when created and call setDefault if it's not nil or an empty string?

@andscoop
Copy link
Contributor Author

andscoop commented Mar 6, 2018

@schnie pushed change that

  • removes gettable and settable props as we discussed offline about them being pointless for now. all configs should be gettable and settable - otherwise we will hardcode them
  • renames meta.go to types.go

@andscoop andscoop merged commit c945fa1 into master Mar 8, 2018
@andscoop andscoop deleted the feature/36-42-config-improvements branch April 18, 2018 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename config.json to astro.json to avoid old CLI conflicts Add --global / -g flags to astro config set/get
2 participants