Skip to content
This repository has been archived by the owner on Mar 2, 2022. It is now read-only.

Add config file parsing #76

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

juga0
Copy link
Collaborator

@juga0 juga0 commented Jan 31, 2018

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 .

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 63.542% when pulling ccb9849 on juga0:feature/config into 839fc5a on TheTorProject:develop.

@coveralls
Copy link

coveralls commented Jan 31, 2018

Coverage Status

Coverage decreased (-3.6%) to 61.002% when pulling 2457b8e on juga0:feature/config into c77c600 on TheTorProject:develop.

Copy link
Member

@meejah meejah left a 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!

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')
Copy link
Member

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

Copy link
Collaborator Author

@juga0 juga0 Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in juga0@e9642bb

@@ -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(),
Copy link
Member

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

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

@juga0 juga0 Mar 23, 2018

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.

Copy link
Member

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.

@juga0 juga0 added this to the simple_working milestone Mar 22, 2018
@juga0 juga0 force-pushed the feature/config branch 2 times, most recently from 342d088 to e9642bb Compare March 23, 2018 09:14
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
@juga0
Copy link
Collaborator Author

juga0 commented Mar 23, 2018

Tests pass but not ready to merge, cli fails

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants