Skip to content
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

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

MehdiNV
Copy link
Contributor

@MehdiNV MehdiNV commented Oct 11, 2023

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) only

Development 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

  • All tests run normally.
  • Added new test case where a TaskNode is created w/ a name but no namespace, to check the namespace 'None' will not be included

Image Example

Before
 
image

After
 
image

Signed-off-by: mehdinv <mehdinv@hotmail.com>
@MehdiNV MehdiNV marked this pull request as ready for review October 11, 2023 15:25
Copy link
Member

@tynandebold tynandebold left a 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.

Comment on lines +414 to +415
if task_node.namespace is not None
else f"kedro run --to-nodes={kedro_node._name}"
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

@merelcht merelcht left a 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?

Comment on lines +414 to +415
if task_node.namespace is not None
else f"kedro run --to-nodes={kedro_node._name}"
Copy link
Member

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.

@tynandebold
Copy link
Member

Yes, good call. Please add a line to the release notes after creating a new section titled # Next Release.

Signed-off-by: mehdinv <mehdinv@hotmail.com>
@MehdiNV
Copy link
Contributor Author

MehdiNV commented Oct 12, 2023

ff0944f

Sounds good! Done just now, ff0944f

RELEASE.md Outdated Show resolved Hide resolved
Co-authored-by: Tynan DeBold <thdebold@gmail.com>
@MehdiNV MehdiNV merged commit c43fb03 into main Oct 12, 2023
19 checks passed
@MehdiNV MehdiNV deleted the fix/1548-missing-pipeline-name branch October 12, 2023 14:13
@tynandebold tynandebold mentioned this pull request Oct 20, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Pipeline Name in kedro-viz Run Command
3 participants