-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
chore(sdk.v2): Implement LoopArgument and LoopArgumentVariable v2 #6755
Conversation
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
Thanks!
"""Extracts the loop item type. | ||
|
||
This method is used for extract the item type from a collection type. | ||
For example:: |
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.
nit: extra :
here (same for below)
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.
We aren't very consistent on this usage. But ::
is used for code block: https://devguide.python.org/documenting/#source-code
That said, the content below isn't strictly code, so removed the extra :
.
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.
Ah I see, thanks for explaining!
if name_override is None: | ||
super().__init__(name=self._make_name(name_code), **kwargs) | ||
else: | ||
super().__init__(name=name_override, **kwargs) |
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.
Does this mean name_override
takes priority over name_code
? or do we expect only one of name_code
and name_override
to appear?
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.
That's a good point. Yes, we expect only one of them to appear. Added a check.
@chensun: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ji-yaqi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is new code not used anywhere, it can't affect kubeflow-pipelines-samples-v2 test. |
Description of your changes:
Checklist: