Code review comment for lp://staging/~tealeg/landscape-client/allow-duplicate-keys-in-config

Revision history for this message
Chris Glass (tribaal) wrote :

This looks good, however I'd like to add some tests to make sure we describe the behavior well:

[1]
Please add a test with keys before and after the duplicate entry, to assert these are not lost (or how they are lost).
Consider:

    [client]
    url = http://example.com/message-system
    computer_title = frog
    computer_title = flag
    log_level = debug

What is config.log_level in this case?

[2]
What happens when more than one keys are duplicates? triplicates?

[3]
+ but the latest defined value should be used.
+ """

This contradicts the test, where the *first* defined value is used.

[4]
+ config_obj = ConfigObj(config_source, list_values=False,
+ raise_errors=False)

Please add a comment explaining what raise_errors does (similar to the one about list_values)

review: Needs Fixing

« Back to merge proposal