Code review comment for ~jslarraz/review-tools:container-add-get-yaml

Revision history for this message
Jorge Sancho Larraz (jslarraz) wrote (last edit ):

Regarding the naming, I don't think that the function name should indicate that it wraps `get_file` as it is an implementation detail that should be transparent for anyone using this function. Otherwise, changing the implementation would require to update the function name, what would break backwards compatibility. It should probable be better included in the documentation. That said, I'll be happy to update the name to `get_file_as_yaml` as it makes more evident that we are actually reading a file from the container and interpreting it as yaml.

Regarding duplicated keys, I just hard-coded them because yaml specification does not allow them and all cases where I started to use the `get_yaml` function, previously checked for duplicated keys with the function `_verify_no_duplicated_yaml_keys`, but I agree that it should be better handled via function arguments.

Working on that!

« Back to merge proposal