-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Resolve Attr Path for List or Dict of Promise #2828
Conversation
Signed-off-by: Mecoli1219 <[email protected]>
Yes you are amazing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2828 +/- ##
==========================================
- Coverage 80.57% 78.87% -1.71%
==========================================
Files 260 196 -64
Lines 23778 20344 -3434
Branches 2618 2622 +4
==========================================
- Hits 19160 16047 -3113
+ Misses 3922 3575 -347
- Partials 696 722 +26 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
@pingsutw I updated the function to remove some redundant code and also support the Dictionary. Could you review it for me? Thx! |
testing this PR, let me merge it. |
Do you want to also add a test for dict[str, type]? |
@Future-Outlier I updated it in the description |
* enable attr path to list Signed-off-by: Mecoli1219 <[email protected]> * enable nested list Signed-off-by: Mecoli1219 <[email protected]> * nit Signed-off-by: Mecoli1219 <[email protected]> * Enable dict support Signed-off-by: Mecoli1219 <[email protected]> --------- Signed-off-by: Mecoli1219 <[email protected]>
Tracking issue
flyteorg/flyte#5856
flyteorg/flyte#5593
Why are the changes needed?
We should support this:
What changes were proposed in this pull request?
If the input is the list, call
resolve_attr_path_in_promise
for each element of that.How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link