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

Allow using commandline options AND/OR config file when running start.py #14

Closed
penyuan opened this issue Jun 4, 2020 · 6 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@penyuan
Copy link
Contributor

penyuan commented Jun 4, 2020

Right now, our start.py needs both commandline options and a .config file to run. Let's update it so that it is more flexible where start.py can get everything it needs from commandline options, an arbitrarily named configuration file at any location, or a combination of both. In the end, the configuration file would be optional but useful.

Also remember to define what happens by default if no options and configuration file are provided.

I suspect this is not the highest priority item right now, but I'm putting this issue here so I will remember to do it someday...

@penyuan
Copy link
Contributor Author

penyuan commented Jun 16, 2020

Per issue #27, reminding myself to add options for logging.

@penyuan
Copy link
Contributor Author

penyuan commented Jun 19, 2020

Options for logging added (including basic checking/exception handling) and the branch iss14 has now finally been merged into the master branch. @jbon can you test the current master branch at commit 64fbd12c to see how it looks to you? Let me know what you think, thanks!

P.S.: Sorry I should not have merged it into master first!!! But can you test anyway?

@penyuan penyuan added the For review when the issue has been fixed and is ready to get reviewed and merged back label Jun 19, 2020
@jbon
Copy link
Contributor

jbon commented Jul 2, 2020

Tested, works well. The only thing that is not clear to me is is the priority between command line arguments and config files item. For the moment it seems that config file has precedence over command line arguments. The other way round would feel more natural to me (I would think the config.json as a "global" config that is pretty much stabel and the command line arguments as a "local" config you change more often)

In any way, I think this should be mentionned in the documentation (data/manual.md, see #31 )

@jbon jbon removed the For review when the issue has been fixed and is ready to get reviewed and merged back label Jul 2, 2020
@penyuan
Copy link
Contributor Author

penyuan commented Jul 6, 2020

Thanks so much for testing. Yes, the functions implemented in this issue should be documented, and I will do so with priority, since I know it is easy to forget things quickly if you don't document well.

As for prioritisation of configuration file versus command line options, I've been thinking about this for the past few days. Most of my experience with command line programs tells me that a user-supplied configuration file will take precedence over any other options. I mean, if the user supplied a configuration file, then presumably that file contains what the user wants.

However, I think your suggestion for "the config.json as a global config that is pretty much [stable]" is interesting. So I propose that we prioritise the files this way: A config.json file that "ships" with the script > A user-supplied configuration file > Everything else (including other command line options).

What do you think?

@jbon
Copy link
Contributor

jbon commented Jul 7, 2020

I am not sure this addresses the point I was trying to make. I just thought that since the config file contains more info than you could reasonably type in a command line (especially if we accept my suggestions in iss27) and a command line is the last thing you touch before starting the execution, it would make more sense to give command line options the priority over the config file. That way, you would have your general config in the config file, generally reflecting the way you want the program to work. But if one time you want to temporarily change one parameter to test something, I guess it would make more sense not tell it to the program via a command line so you don't to touch your config file. But that's maybe just the way I work and may not reflect general practices.

@penyuan
Copy link
Contributor Author

penyuan commented Jul 22, 2020

Per our discussion on 2020-07-17, the following order of precedence is suggested (based on what seems to be the industry standard):

hard-coded defaults < environment variables < user-supplied configuration file < command line arguments/options

Where items on the right would override options on the left.

@penyuan penyuan closed this as completed Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants