Allows for multiple -s|--section options - #148
Conversation
Signed-off-by: Mateus Caruccio <mateus.caruccio@getupcloud.com>
|
Hey @mattrobenolt any chance of this to be merged? |
|
You literally opened this yesterday, 24 hours ago. |
|
Oh, my apologies. I wasn't expecting you to respond that soon. |
Signed-off-by: Mateus Caruccio <mateus.caruccio@getupcloud.com>
mattrobenolt
left a comment
mattrobenolt
left a comment
There was a problem hiding this comment.
The feature makes sense and I want it, but this needs another round. The inline comments are the blocking stuff, and CI is red right now because of the first one.
One design question before you respin: is merging the sections into one flat namespace the behavior you're after? The description says "filtered", which sounds more like keeping the top-level dict and narrowing it to the named sections, something like data = {k: data[k] for k in opts.section}. That keeps {{ a.x }} and {{ b.y }} namespaced, has no key collisions, and the mapping check goes away entirely. Flattening means two sections iwth the same key silently clobber each other, which feels like a footgun. If merge is the deliberate choice, cool, but let's make it deliberate.
Also needs tests for the new behavior.
| if opts.section: | ||
| section = opts.section | ||
| # Use specified sections if needed | ||
| if len(opts.section) == 1: |
There was a problem hiding this comment.
This crashes when -s isn't passed at all. action="append" without a default leaves opts.section as None, and len(None) is why CI is fully red right now. The failing tests never pass -s, so the default path is just dead. default=[] on the argument fixes it.
| merged_data = {} | ||
| for k, v in data.items(): | ||
| if k in opts.section: | ||
| if not isinstance(v, Iterable): |
There was a problem hiding this comment.
Strings are Iterable, so this guard doesn't guard anything. A section whose value is a string sails through this check, then dict.update() explodes with ValueError: dictionary update sequence element #0 has length 1; 2 is required. You want collections.abc.Mapping here, and the error message means mapping, not iterable.
| elif len(opts.section) > 1: | ||
| # for multiple values, all must be iterables | ||
| merged_data = {} | ||
| for k, v in data.items(): |
There was a problem hiding this comment.
Walk opts.section instead of data.items(). Two problems as written: a typo'd section name silently succeeds (single -s raises unknown section, multi just renders with the data missing), and merge order follows the data file's key order instead of the command line, so -s a -s b and -s b -s a produce identical output when keys collide. Iterating the flags fixes both.
|
Hello Matt. I'm very busy right now, but am planning to come back ASAP. As you stated before, this is not working as intended. |
2 participants
This PR adds support for multiple
-s|--sectionoptions to be provided in the command line.This allows for more than one section being filtered for the data context.