Conversation
tfoote
left a comment
tfoote
left a comment
There was a problem hiding this comment.
Thanks this has been something that I've wanted to do overall. I have a behavior tweak request.
And to merge this we definitely need to add unit tests.
Why did you pick YAML versus using INI or TOML?
And to that matter Potentially ConfigArgParse might be a good solution to this problem: https://pypi.org/project/ConfigArgParse/ and will provide the different layers automatically for us.
| parser.add_argument('image') | ||
| parser.add_argument('command', nargs='*', default='') | ||
| parser.add_argument('--config', help='''Optional yaml file to handle command line arguments | ||
| (except positional args) as a config file. This config will override any other command line |
There was a problem hiding this comment.
Please change the order of precedence here. The command line should be higher priority than the config file. It's closer to the user and has been explicitly typed out on the command line so should take precedence.
This will be really convenient to have standard configurations. And then you can override or extend for a specific need on the commandline reusing the saved config file that's closest.
There was a problem hiding this comment.
Yeah I 100% agree. I spent a little while fiddling with it before submitting this PR but couldn't get it to work and I gave up. Do you know of a way to determine which args from argparse are explicitly passed? I've seen a few methods out there (namely creating a second namespaced parser to compare against), but none of them work as-is with the non-trivial argument options that rocker has so it requires some more digging.
One way I had initially thought of was to check for default values in the argument list, but if you explicitly want to override a config option with a default value, you won't be able to. If you don't have any immediate ideas, I'll find another way to make it work
| # Load config file if provided | ||
| if args.config: | ||
| with open(args.config, 'r') as f: | ||
| config = yaml.safe_load(f) |
There was a problem hiding this comment.
I feel like we should be doing some validation here.
And providing some visibility as to what's being loaded and from where as well. Think about how one would debug a typo in a config value or confirm that something is being selected.
Habit. I don't have a strong preference so I'll update that to one of your suggestions |
Can you see how ConfigArgParse would work for this case? It looks like the If the documentation is as I think we might be able to just use Or maybe using the file references in ArgParse: https://docs.python.org/3/library/argparse.html#fromfile-prefix-chars It would be great to leverage other's work in this area. |
|
@tfoote I've had a few partial implementations sitting around on my machine for this, but I'd like to make an effort to actually clean it all up and get it working upstream. |
2 participants
Added a command line option to pass a config file with the command line arguments. I've been using this for a while and have found it handy and I didn't see a way that already existed to allow for this.