feat: support lists in 'resolve_keyed_by' #608
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ahal
force-pushed
the
push-zptlxmssuqnn
branch
2 times, most recently
from
5daf787 to
8c4f563
Compare
jcristau
requested changes
Nov 26, 2024
hneiva
reviewed
Nov 27, 2024
|
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
jcristau
reviewed
Jan 10, 2025
ahal
force-pushed
the
push-zptlxmssuqnn
branch
from
4ed10f0 to
89a1539
Compare
bhearsum
approved these changes
Jan 20, 2025
Comment on lines
+23
to
+24
| ), | ||
| ), |
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?
jcristau
reviewed
Jan 20, 2025
| task["label"], | ||
| platform=platform, | ||
| ) | ||
| yield task |
There was a problem hiding this comment.
Suggested change
| yield task | |
| yield new_task |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I tested this in Gecko and it resulted in an identical full graph