added support for module value sources + pyflake cleanup #63

Closed
wants to merge 1 commit into
from

2 participants

@twobraids
Mozilla member

This revision adds the use of python modules as value sources. The name of a module in a package can now be used instead of a configuration file:

.../config/commonconfig.py:
    HBaseHost = 'localhost'
    HBasePort = 9090

.../some/dotted/module.py:
    from config.commonconfig import HBaseHost as hbase_host
    from config.commonconfig improt HBasePort as hbase_port
    stderr_logging_level = 30

$ someconfigman_app --admin.conf=some.dotted.module --help
   someconfigman_app 1.0
      --hbase_port
         the port that HBase needs to connect to (default:9090)
      --hbase_host
        the host on which HBase is found (default:localhost)
      --stderr_logging_level
        the logging level for stderr (default: 30)
@peterbe peterbe commented on the diff Dec 4, 2012
configman/tests/test_val_for_mappnig.py
@@ -0,0 +1,83 @@
+# ***** BEGIN LICENSE BLOCK *****
@peterbe
peterbe added a line comment Dec 4, 2012

File name is "mappnig.py"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on the diff Dec 4, 2012
configman/value_sources/for_mapping.py
)
+def get_symbols_from_module(a_module):
+ return dict((key, getattr(a_module, key)) for key in dir(a_module)
+ if isinstance(key, basestring))
@peterbe
peterbe added a line comment Dec 4, 2012

dir(some_module) always returns a list of strings.

One thing we could do is something like ... if not key.startswith(key)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on the diff Dec 4, 2012
configman/value_sources/__init__.py
@@ -130,8 +130,12 @@ def wrap(value_source_list, a_config_manager):
# if you have specified an admin.conf value that is different
# from the default, the raise hell if the file doesn't exist
default = a_config_manager._get_option('admin.conf').default
- if a_source and a_source != default and not os.path.isfile(a_source):
- raise IOError(a_source)
+ if (a_source and a_source != default
+ and not os.path.isfile(a_source)):
+ try:
+ a_source = __import__(a_source)
+ except ImportError:
+ raise IOError(a_source)
@peterbe
peterbe added a line comment Dec 4, 2012

Why?
If I got an ImportError it would suspect I mispelled the module path because it couldn't find the file. When in fact it found the file perfectly fine but couldn't import it.

If anything, I would create a new exception class to dedicate to this.

Either way, we need logging there if we decide to swallow the ImportError.

@twobraids
Mozilla member
twobraids added a line comment Dec 4, 2012

I think the new exception class is the way to go. As it stands, if you use an ini file and mistype it, you'll get the expect IOError. It would make no sense if you were to type 'myconfig.ini' and get an ImportError

@peterbe
peterbe added a line comment Dec 4, 2012

My bad. Doing __import__('filethatdoesntexist') will raise an ImportError too. My worry was that the import might have worked initially but within the imported file there are subsequent ImportErrors.

Fine. Let's leave it but can we add some sort of logging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on the diff Dec 4, 2012
configman/value_sources/for_mapping.py
@@ -52,10 +59,14 @@ def __init__(self, source, the_config_manager=None):
self.always_ignore_mismatches = True
elif isinstance(source, collections.Mapping):
self.always_ignore_mismatches = False
+ elif isinstance(source, types.ModuleType):
+ self.always_ignore_mismatches = True
+ source = get_symbols_from_module(source)
@peterbe
peterbe added a line comment Dec 4, 2012

shouldn't that be source = DotDict(get_symbols_from_module(source))

Otherwise, in the case of Mapping' type instances you're going to doDotDict(source is already a mapping thing)` a couple of lines later.

@twobraids
Mozilla member
twobraids added a line comment Dec 4, 2012

all the types get wrapped in DotDict on line 68 at then end of the method. doing here would be doing it twice.

@peterbe
peterbe added a line comment Dec 4, 2012

It worked before with self.source = source.

Perhaps we should do:

if not isinstance(source, collections.Mapping):
    source = DotDict(source)
self.source = source
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on the diff Dec 4, 2012
configman/value_sources/__init__.py
@@ -130,8 +130,12 @@ def wrap(value_source_list, a_config_manager):
# if you have specified an admin.conf value that is different
# from the default, the raise hell if the file doesn't exist
default = a_config_manager._get_option('admin.conf').default
- if a_source and a_source != default and not os.path.isfile(a_source):
- raise IOError(a_source)
+ if (a_source and a_source != default
+ and not os.path.isfile(a_source)):
+ try:
@peterbe
peterbe added a line comment Dec 4, 2012

Are there any tests that get here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe

r+ if you add documentation. We have to be stringent about that even if it's boring to do.

@twobraids
Mozilla member

this is bit rotted and needs to be redone - withdrawing for now

@twobraids twobraids closed this Oct 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment