-
Notifications
You must be signed in to change notification settings - Fork 6
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
Job nested process workflow #748
Conversation
8025528
to
2504056
Compare
… schema references
…aver into job-nested-process-workflow
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #748 +/- ##
==========================================
+ Coverage 87.07% 87.40% +0.33%
==========================================
Files 80 80
Lines 20395 21312 +917
Branches 2774 2954 +180
==========================================
+ Hits 17758 18627 +869
- Misses 1900 1921 +21
- Partials 737 764 +27 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
# type: (Dict[str, Any]) -> List[colander.SchemaNode] | ||
self.children = [node.clone() for node in ExecuteProcessParameters().children] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't confusing calling it a bind if it's not a shallow copy ?
Shouldn't it be self.children = list(ExecuteProcessParameters().children)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limitation from the parent colander
library that called the method bind
. Nested nodes have to be cloned, otherwise you could have by-ref conflicting updates if similar definitions are used simultaneously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test in ef93c74
(#748) so you can get a better understanding of the feature and its behavior.
process: <uri>
construct #747