[WIP] Move config file handling to external packages#3582
[WIP] Move config file handling to external packages#3582fionera wants to merge 2 commits intorclone:masterfrom fionera:rework-config-package
Conversation
| return s.GetStringDefault(name, "") | ||
| } | ||
|
|
||
| func (s *section) GetStringDefault(name string, default_ string) string { |
There was a problem hiding this comment.
don't use underscores in Go names; method parameter default_ should be default (from golint)
| ) | ||
|
|
||
| const ( | ||
| RemotesPrefix = "remotes" |
There was a problem hiding this comment.
exported const RemotesPrefix should have comment (or a comment on this block) or be unexported (from golint)
| func (c *viperConfig) GetRemotes() []string { | ||
| var remotes []string | ||
| remoteEntries := viper.GetStringMap(RemotesPrefix) | ||
| for key, _ := range remoteEntries { |
There was a problem hiding this comment.
should omit 2nd value from range; this loop is equivalent to for key := range ... (from golint)
| return s.v.GetString(GetConfigKey(s.basePath, name)) | ||
| } | ||
|
|
||
| func (s *section) GetStringDefault(name string, default_ string) string { |
There was a problem hiding this comment.
don't use underscores in Go names; method parameter default_ should be default (from golint)
| func (s *section) GetStringDefault(name string, default_ string) string { | ||
| if s.v.IsSet(GetConfigKey(s.basePath, name)) { | ||
| return s.v.GetString(GetConfigKey(s.basePath, name)) | ||
| } else { |
There was a problem hiding this comment.
if block ends with a return statement, so drop this else and outdent its block (from golint)
| "context" | ||
| "flag" | ||
| "fmt" | ||
| "github.com/rclone/rclone/fs/config/provider/goconfig" |
There was a problem hiding this comment.
File is not goimports-ed (from goimports)
The config system was designed to be replaceable by setting these global variables Lines 16 to 28 in 77a520c Now whether that is enough configuration I'm not sure since I don't think anyone used that feature... That doesn't work with I think your patch goes one step further so
I thought about using viper at some point but decided it would be too difficult to retro-fit. I don't really know how it works though - how are you using it? |
yes I saw that but since I did not wanted to use a string as storage, it wasnt enought. I wanted to use configs like this: https://i.imgur.com/QxaJpAt.png
Yes I completely moved the file handling to a different package and made it switchable. The access is now over interfaces which also makes mocking far easier.
It is basicly a wrapper around yaml, toml etc. |
I'm trying to understand in what way wasn't it enough? What you mean by "use a string as storage"? |
|
I needed a map value and since I hate using CLIs I wanted to be able to still write the config manually. In the screenshot is an example of this |
|
@ncw Would it be better to split this into multiple smaller PRs? Like moving the Config System itself into a different file, abstracting it more etc.? |
Sorry I've been a bit distracted trying to firm up the 1.50 release! Yes smaller PRs would be much easier to review. Maybe first step you could post a |
Sure thing :) Will close this and open an Issue |
3 participants
What is the purpose of this change?
Replace the current goconfig handling with an abstraction to allow use of other config types. I only did Search and Replace for the tests, so I dont know if anything works yet. Also I dont know if this is even a good idea. I use viper for a fork I maintain to have a map[string][]string type for remotes and more flexible handling of these, so I thought maybe upstream it
Was the change discussed in an issue or in the forum before?
I think I mentioned it somewhere 😄
Checklist