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

Clarify error handling and config file search techniques in documentation #32

Closed
untoldone opened this issue Dec 12, 2014 · 9 comments
Closed

Comments

@untoldone
Copy link

I included a few specific issues I had to figure out in issue #6 that has been closed, but I hadn't seen any specific fixes in the docs for the issues listed. I'm fine with the bug being 'wont fixed' but I just wanted to be sure the specific issues I had were bubbled up in case they were seen as just a 'me too' post on the thread. From #6:

I couldn't get this working on my first try out of the box and no feedback/ detailed errors (beyond generic io errors) from the apis makes figure this stuff out even harder

In my particular case, I assumed two things that were not obvious to me without reading the code:

  • the current working directory is not included in configPaths by default.
  • the config file is required to have the extension of the file type even though you can specify through SetConfigType. I assumed the search code would have included looking at configName rather than just configName+"."+ext for each known extension

In addition, it would have been helpful for the example that does exist in the readme to properly check for errors loading the file to make it clear that thats how the API works without needing to dig into the source (as is the norm in go) e.g:

viper.SetConfigName("config") // name of config file (without extension)
viper.AddConfigPath("/etc/appname/")   // path to look for the config file in
viper.AddConfigPath("$HOME/.appname")  // call multiple times to add many search paths
err := viper.ReadInConfig() // Find and read the config file
if err != nil {
  fmt.Println("Failed to read config", err)
}
@kzvezdarov
Copy link
Contributor

Implemented some of your suggestions in #55 . Probably will add the optional filename extension as well but need to give it a bit more thought.

@untoldone
Copy link
Author

Appreciate the followup! Thanks for looking into this.

@kzvezdarov
Copy link
Contributor

Merged the fix for pwd not being included by default.
I will hold off of including implicit config file extensions for now. SetConfigType does not set only the filetype of the local config file, it is also used to get the expected format of the data returned from the remote source. That mechanism can use some work, so I'd rather first improve it and then add implicit file extensions.
Keeping this open for now as the implicit config type issue is not fixed yet.

@untoldone
Copy link
Author

Sounds good to me! Thanks for looking into this.

On Fri, Apr 3, 2015 at 6:00 AM, Kiril Zvezdarov notifications@github.com
wrote:

I will hold off of including implicit config file extensions for now.
SetConfigType does not set only the filetype of the local config file, it
is also used to get the expected format of the data returned from the
remote source. That mechanism can use some work, so I'd rather first
improve it and then add implicit file extensions.
Keeping this open for now as the implicit config type issue is not fixed
yet.


Reply to this email directly or view it on GitHub
#32 (comment).

@didenko
Copy link
Collaborator

didenko commented May 22, 2015

A vote against including the workdir by default: #55 (comment)

@erichiller
Copy link

I just found this issue, and I don't have a strong argument for or against including WD in the path by default, it should at least be a little more clear in the docs, the aforementioned:

viper.SetConfigName("config") // name of config file (without extension)
viper.AddConfigPath("/etc/appname/")   // path to look for the config file in
viper.AddConfigPath("$HOME/.appname")  // call multiple times to add many search paths
err := viper.ReadInConfig() // Find and read the config file
if err != nil {
fmt.Println("Failed to read config", err)
}

was better than all the other docs I found on the rest of the internet, as I did not want to set the working directory statically. I simply did:

viper.AddConfigPath(".")        // path is not known or set by default

If that was just dropped into the README.md, it might help a ton, no need to change the src I think.

@didenko
Copy link
Collaborator

didenko commented Nov 3, 2015

@variab1e, the issue of WD used by default caused plenty of confusion and was resolved in #73 by removing it from a default search sequence. Your way of explicitly adding WD is proper.

@didenko
Copy link
Collaborator

didenko commented Nov 3, 2015

Added PR #128 - does it address your concerns?

@erichiller
Copy link

Yes, I think this is perfectly clear.

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

No branches or pull requests

4 participants