mkideal / cli

CLI - A package for building command line app with go
MIT License
732 stars 43 forks source link

Need to loosen the tight control of the config name #49

Closed suntong closed 5 years ago

suntong commented 6 years ago

With the introduction of $__EXEC_FILENAME the config can go with the executable now. However, now the problem is that the config MUST be named precisely according to the executable -- if people want to name it to something else, e.g., like this, things will be broken again.

I think it is time to loosen the tight control of how the config should be named, and give the freedom back to the user. I.e., even when people name their config file to whatever they like, the self-config file should still work.

Please consider.

PS. for my daily job, I have 3 or 4 different tools doing different things, but they all rely on a central config file. I put the exes together and they all use the same config file. I know this might be a rare case, but I do consider it a very valid one.

Please reconsider -- people need the freedom, not the unnecessary restriction. Thx.

mkideal commented 6 years ago

What do you mean by allowing you to read your own named configuration file and can the configuration file always be in the same directory as the executable?

mkideal commented 6 years ago

Now, you can set config name as following

Self  *rootT `cli:"c,config" usage:"config file\n" json:"-" parser:"jsonfile" dft:"$__EXEC_PATH/yourname.json"`
suntong commented 6 years ago

Ha, that's clever.

suntong commented 6 years ago

Hmm... that means I cannot override the provided default that comes with the exe, even if I have a configuration file in the current directory.

Sometimes it is necessary to use the configuration file in the current directory to override the provided default that comes with the exe. So I'm thinking to read current directory then fall back to the EXEC_PATH is a better option, when it comes to providing the defaults, because there will be times that the provided default that comes with the exe need to be overridden.

Please consider. thx.

suntong commented 6 years ago

I think the best way to architect this, is to have a separated function specifically to handle read configuration, not to directly using the ReadJSONFromFile method, because providing the defaults from the configuration file is really what this self-config is all about, which does require the versatility of providing the defaults at different levels.

Ideally, this new ReadConfigFile method will allow the configuration file that comes with the exe to define all the defaults (say, setting1 to setting9 ), while the user overriding configuration file, be it under ~/.config/ or at the current directory only need to override just a few (e.g., setting3 to setting7, etc)

I think that's the best architect for this.

Moreover, please bear in mind that Debian/Ubuntu has a very strict policy where the configuration file should reside -- the exe files are under \usr, but the configuration file should be under \etc. Please consider this scenario as well when architecting this, as I do wish that CLI tools based on this module can be packed officially into Debian/Ubuntu easily.

Thanks for consideration.

mkideal commented 6 years ago

I don't quite understand what you mean. Can you submit a PR?

suntong commented 6 years ago

sorry for responding late. I've been out of town and just get back. Will find some time in two weeks time to look into to the fix/PR.

suntong commented 6 years ago

The main point is to duplicate the existing ReadJSONFromFile function to an almost identical function, for reading the configuration file only. Everything else in my above message is implementation details.

The question is, would you think such "duplication" acceptable?
The differences between the two is that the one for reading the configuration file has several built-in locations to look for the file, namely:

in the above order.

suntong commented 5 years ago

ping,

would you think such "duplication" acceptable, @mkideal?

Basically,

Take a look at all the changes I made in https://github.com/mkideal/cli/issues/40

especially, https://github.com/go-easygen/cli/commit/e506bf61c8120e8cfda3276ddf4075864f3da6fb?diff=unified

Which is quite a simple change, however, you thought

It's not a good idea for changing ReadJSONFromFile method.

So I'm thinking to duplicate ReadJSONFromFile with all my changes in https://github.com/mkideal/cli/issues/40, and use it for reading self-config .json files (only).

Would that be acceptable?

mkideal commented 5 years ago

Yes, it's acceptable!

suntong commented 5 years ago

Great, I'll find some time to send in the PR -- need to fix my weird DNS & google drive access problem I'm facing now first...

suntong commented 5 years ago

OK, I've done all the leg works and have added ReadCfgJSONFromFile():

https://github.com/mkideal/cli/compare/master...suntong:master

However, it is not working as expected. I.e., parser:"jsonfile" is not actually calling it (verifying steps documented in #40). But I don't know the cli internal logic well enough to fix it.

Would you take a look and make it work please? I'll create a half-baked PR and please take over from there -- https://github.com/mkideal/cli/pull/52 Thx.

mkideal commented 5 years ago

See example 032. Is it what you need?

Test procedure:

cd _examples/032-jsoncfg/
go build
./032-jsoncfg
# output
# {
#    "A": "a string",
#    "B": 12,
#    "C": true
# }
cd ../
./032-jsoncfg/032-jsoncfg
# output
# {
#    "A": "a string",
#    "B": 12,
#    "C": true
# }

Your mistake is in file parser.go. You should register a new parser jsoncfg instead of jsonfile

suntong commented 5 years ago

YEP! Perfect! That's exactly what I want!

Thanks a lot -- finally I am able to switch all my cli based programs back. Hooray~