Code review comment for lp:~vila/awsome/config-with-tests

+Default values can be acquired from environment variables for backwards
+compatibility during a transition period.

So, I disagree somewhat here as we've discussed before. For development, I like not having to copy secret values into a config file to test against different clouds, and I don't see how that changes.

+ if not isinstance(value, str):
+ raise AssertionError(
+ 'Callable default values should be strings')

Assertions without any context at all are a bit annoying.

+class MultilineVersion(argparse.Action):

Slightly confusing copying this in in this mp as it's only removed from bin/awsome in the followup, but not harmful.

+def bool_from_string(string):
+ val = None
+ if isinstance(string, str):
+ try:
+ val = _valid_boolean_strings[string.lower()]
+ except KeyError:
+ pass
+ return val

I'd use a more compact spelling avoiding the isinstance check:

    try:
        return _valid_boolean_strings[string.lower()]
    except (AttributeError, KeyError):
        return None

Or `str.lower(string)` catching TypeError if you really want to be picky about the type.

+def set_env_variable(name, value):
...
+def overrideEnv(test, name, value):
...
+def overrideAttr(test, obj, attr_name, new=_unitialized_attr):

I'd stick all these in awsome.tests so we have a one place with all the extra testing bits we needed to bring in.

+ def test_first_default_value_from_env_wins(self):
+ opt = config.Option('foo', default='bar',
+ default_from_env=['NO_VALUE', 'FOO', 'BAZ'])

Not isolated from NO_VALUE being set in the test process.

+ self.assertNotEquals(None, option_help)
+ # Come on, think about the user, he really wants to know what the
+ # option is about
+ self.assertIsNot(None, option_help)

First assertion is meaningless given the second, nothing ordinary compares equal to None except itself.

+ def test_error(self):

Not wild about this test, overriding standard streams and catching SystemExit is a little dodgy in tests. Would prefer some refactoring to avoid that, but I suspect argparse doesn't make our lives easy here so it's not that important.