feat: support lists in 'resolve_keyed_by' #608
Conversation
ahal
force-pushed
the
push-zptlxmssuqnn
branch
2 times, most recently
from
5daf787 to
8c4f563
Compare
|
Moving this back to draft to get it out of review queues. |
|
Oops, I forgot about this PR.. I'll get it into shape soon |
ahal
force-pushed
the
push-zptlxmssuqnn
branch
from
8c4f563 to
4ed10f0
Compare
ahal
force-pushed
the
push-zptlxmssuqnn
branch
from
4ed10f0 to
89a1539
Compare
| ), | ||
| ), |
There was a problem hiding this comment.
Some tests for error / corner cases would be welcome. Eg:
container: {"a": [{"b": 1}, {"c": 2}]}
subfield: a.b
| @transforms.add | ||
| def resolve_artifact_paths(config, tasks): | ||
| for task in tasks: | ||
| resolve_keyed_by(task, "worker.artifacts.path", task["label"]) |
There was a problem hiding this comment.
I don't want to bikeshed on this too much, so this is certainly not a blocker, but I have to admit that I find it a bit unintuitive that .path accesses multiple list items (in addition to access plain dict keys). I wonder if jq-style syntax would be preferable here? Eg: worker.artifacts[].path?
There was a problem hiding this comment.
I agree with you. I'll mark this as draft until I get a chance to implement a [] syntax.
| task["label"], | ||
| platform=platform, | ||
| ) | ||
| yield task |
There was a problem hiding this comment.
| yield task | |
| yield new_task |
|
I didn't end up needing this for the thing I was working on at the time, so I've neglected it again.. but I'll try and get this landed at some point. |
I tested this in Gecko and it resulted in an identical full graph