Conf dir #21
Conf dir #21
Conversation
dubo-dubon-duponey
force-pushed
the
conf_dir
branch
from
2dc1086 to
127f2ab
Compare
|
Rebased. |
main.go
Outdated
| flag.StringVar(&config.Config.DeviceName, "n", "goplay", "Specify device name") | ||
| flag.StringVar(&ifName, "i", "eth0", "Specify interface") | ||
| flag.Int64Var(&delay, "delay", 0, "Specify hardware delay in ms") | ||
| flag.StringVar(&config.Config.PulseSink, "sink", config.Config.PulseSink, "Specify Pulse Audio Sink - Linux only") | ||
| flag.Parse() // after declaring flags we need to call it | ||
|
|
||
| config.Config.Load() | ||
| config.Config.Load(configurationBaseDir) |
There was a problem hiding this comment.
If configuration load returns an error, it should panic (no cwd directory, nothing will work)
There was a problem hiding this comment.
Good point. Fixed.
Signed-off-by: dubo-dubon-duponey <[email protected]>
dubo-dubon-duponey
force-pushed
the
conf_dir
branch
from
127f2ab to
8d1ccb2
Compare
|
@AlbanSeurat controller filestorage will also write in the "goplay" location, so, that should be patched as well I guess. Data is not configuration though, so, I was thinking about separating these:
Let me know your thoughts. |
dubo-dubon-duponey
force-pushed
the
conf_dir
branch
3 times, most recently
from
642fd42 to
0da73ef
Compare
Signed-off-by: dubo-dubon-duponey <[email protected]>
dubo-dubon-duponey
force-pushed
the
conf_dir
branch
from
0da73ef to
1358c1d
Compare
|
Ok, done:
Also modified main so that certain values passed on the command line do override stored config (sink and data location). This is a bit more extensive than I thought, sorry about that. |
| // Load the possibly existing config | ||
| err := config.Config.Load(configurationBaseDir) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
I would use log.panicf to have a meaningful message
| } | ||
| if flagsConfig.PulseSink != "" { | ||
| config.Config.PulseSink = flagsConfig.PulseSink | ||
| } |
There was a problem hiding this comment.
It start to be a good case to create a new file with all config related code
There was a problem hiding this comment.
Happy to give it a go if you are interested and refactor this proper.
Question though: are you attached to the idea that the config path must contain the DeviceName? IMHO this is problematic for a number of reasons (filesystem reserved chars vs. mDNS type, duplication of DeviceName in the path, on the command-line, and inside the config file)
There was a problem hiding this comment.
Not at all attached, the whole focus for me now is to have good sound and audio sync.
I am happy with a better configuration/usability if you want to help
There was a problem hiding this comment.
Agreed what matters most is good sound and audio sync :) - keep it coming!
Happy to help with the rest (build, packaging, UX) and then just feel free to pick whatever makes sense for you and the project.
Thanks again for doing this project - very much appreciated :)
|
Putting this on pause for now, pending more work. |
Fix #20
New flag -c allows one to point to a base directory where configuration will live.
If unspecified, pwd will be used, which should match the previous behavior.
This is just a quick suggestion at this point, but I thought I would send it over early for feedback / modifications.