-
Notifications
You must be signed in to change notification settings - Fork 278
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
support global config (e.g. for company-wide settings) #233
Conversation
Would you also document it in the README? Otherwise, LGTM! @chavacava, would you take a look as well? |
config.go
Outdated
excludeUsage = "list of globs which specify files to be excluded (i.e. -exclude foo/...)" | ||
formatterUsage = "formatter to be used for the output (i.e. -formatter stylish)" | ||
) | ||
flag.StringVar(&configPath, "config", "", configUsage) | ||
|
||
var globalConfigPath 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.
I propose to rename globalConfigPath
into defaultConfigPath
.
To simplify the reading of the init()
function we can also factor out the calculation of the default path into a helper function like
func buildDefaultConfigPath() string {
var result string
if homeDir, err := homedir.Dir(); err == nil {
result = filepath.Join(homeDir, "revive.toml")
if _, err := os.Stat(result); err != nil {
result = ""
}
}
return result
}
And just write
defaultConfigPath := buildDefaultConfigPath()
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.
I had similar sentiments. But, I didn't have a good idea about where to put the helper function.
Let me know what you think.
@chavacava Your changes have been implemented (logic moved to helper function).
@mgechev I have added documentation to the README
. Thanks for reminding me.
@mgechev I'm not sure how long you want to wait on @chavacava but I vote for closing #232 and merging #233. |
LGTM |
Addresses #232.