configglue team mailing list archive

[Michael Nelson <michael.nelson@xxxxxxxxxxxxx>]

Review: Approve
Great, thanks for the help getting the tests going (sheesh, the joy of non-db tests). So I setup a virtualenv with mock and that worked.

We chatted about the fact that it doesn't really make sense that we're even parsing a string version of a dict here (as no such thing exists in the config files anyway) and you were going to check if there's an easy way to avoid that (if I understood correctly).

Just rewording that: If I have not specified a value for this option in my config, then we should not be parsing anything at all for this item, but simply using the default value (rather than turning the default value into a string to then re-parse).

So if that's not fixable in this branch, let's get it landed as is, but make it clear... so can you please create a bug and XXX it in the code here (in parser).

Also, I think the code would be slightly clearer if you moved the original declaration of is_default_value into an else-clause so the code read something like:

{{{
if (is_dict_option or is_dict_lines_option):
    try:
        is_default_value = (
            ast.literal_eval(value) == option_obj.default)
    except:
        is_default_value = False
    ...
else:
    is_default_value = value == option_obj.to_string(option_obj.default)
}}}

What do you think? Either way, I think just the XXX is required. I'll add this as an approve vote but not approve the MP so you can make your decision and land it when ready.




-- 
https://code.launchpad.net/~ricardokirkner/configglue/parse-default-dict-value/+merge/65840
Your team Configglue developers is requested to review the proposed merge of lp:~ricardokirkner/configglue/parse-default-dict-value into lp:configglue.