-
Notifications
You must be signed in to change notification settings - Fork 110
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
Issue #1548: Fix improper display of 'run command' text #1569
Conversation
Signed-off-by: mehdinv <mehdinv@hotmail.com>
Signed-off-by: mehdinv <mehdinv@hotmail.com>
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.
Nice one! This fixes the problem.
I've left a small question about style and will let @rashidakanchwala or @ravi-kumar-pilla answer it.
if task_node.namespace is not None | ||
else f"kedro run --to-nodes={kedro_node._name}" |
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.
Is this the way we normally write if/else
statements in the codebase?
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.
Yes we do use this style.
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! 👍
Should this be added to the release notes?
if task_node.namespace is not None | ||
else f"kedro run --to-nodes={kedro_node._name}" |
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.
Yes we do use this style.
Yes, good call. Please add a line to the release notes after creating a new section titled |
Signed-off-by: mehdinv <mehdinv@hotmail.com>
Co-authored-by: Tynan DeBold <thdebold@gmail.com>
Description
Resolves #1548
Task nodes that have no namespace, but a name present, will display their run command as
...None.(Node_name)
.The correct display should just be the
...(Node_name)
onlyDevelopment notes
Purposely made changes very light to avoid unintentional bugs around the code (still unfamiliar with code base, so don't want to make large changes I can't predict the effects of).
Primary change for this PR is just to the text display; tells it to ignore 'None' values.
QA notes
Image Example
Before
After