-
Notifications
You must be signed in to change notification settings - Fork 32
Add config file parsing #76
base: develop
Are you sure you want to change the base?
Conversation
ff90b33
to
553d5bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a nicer way with Click to do the "defaults from config file" (see inline comment). Otherwise LGTM!
bwscanner/configutil.py
Outdated
def copy_config(cfg_path, cfg_default_path=None): | ||
if cfg_default_path is None: | ||
cfg_default_path = os.path.join(os.path.dirname(os.path.dirname( | ||
os.path.abspath(__file__))), 'data', 'config.ini') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this sort of thing, it's sometimes nicer to use pkg_resources
(so that it still works if you've packaged a wheel or zip). See http://peak.telecommunity.com/DevCenter/PythonEggs#accessing-package-resources -- and also this interacts with setup.py
, detailed here https://packaging.python.org/tutorials/distributing-packages/#package-data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in juga0@e9642bb
bwscanner/scanner.py
Outdated
@@ -29,18 +38,27 @@ def __repr__(self): | |||
pass_scan = click.make_pass_decorator(ScanInstance) | |||
|
|||
|
|||
@click.group() | |||
# FIXME: change all options to take defaults from CTX, ie config file? | |||
@click.group(context_settings=CTX) | |||
@click.option('--data-dir', type=click.Path(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, instead of this and the CTX.get() stuff below, I think you can: re-define the entry-point as say start
and make that a method like:
def start():
config = read_config(os.path.join(DATA_DIR, CONFIG_FILE))
return cli(default_map=config)
See http://click.pocoo.org/5/commands/#overriding-defaults too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, by reading the docs is not clear to me where start
is supposed to be call.
By the "stuff below" you mean all the default=
in the options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You meant something like juga0@388e824#diff-091411b9c979317818f6582241ab8284?
It doesn't seem simpler/nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I meant. I haven't actually done this config-file-based defaults myself, just basing it off Click's docs. I wouldn't expect you'd have to do that "for k, v in ...default_map" thing either based on what Click says but .... I can play with it a bit myself later.
342d088
to
e9642bb
Compare
instead of using __file__ as it would fail when packed in a zip. Include the config file in the package adding it to MANIFEST.in
* Create start function to call cli with parsed config * Parse bw_files section * Convert parsed config values to their type
Tests pass but not ready to merge, cli fails |
The intention here is to read default configuration variables from a file and overwrite them when they are pass as command line arguments. I'm not sure how to integrate ConfigParser in a nice way click default_map and pass_context .