-
Notifications
You must be signed in to change notification settings - Fork 895
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
Add tests to all new project creation flows #3462
Conversation
Signed-off-by: Ahdra Merali <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>
kedro/framework/cli/starters.py
Outdated
@@ -516,9 +516,10 @@ def _get_extra_context( # noqa: PLR0913 | |||
NUMBER_TO_TOOLS_NAME[tool] | |||
for tool in _parse_tools_input(tools) # type: ignore | |||
] | |||
extra_context["tools"] = str(extra_context["tools"]) | |||
if extra_context.get("tools"): |
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.
Extra condition to catch:
- User prompts tools:"none"
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.
What does this additional check do on top of this check above?
tools = extra_context.get("tools")
if tools:
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.
If the user goes through the prompts and selects none, _parse_tools_input(tools)
on line 517 will return an empty list, which was causing the discrepancy of none resulting in either []
or ['None']
being returned. This additional check ensures this is handled without adding {"none":'None'}
to NUMBER_TO_TOOLS_NAME
which felt incongruous (as "none" isn't a number or tool selection)
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 think I understand the idea, but it seems somewhat tricky and not immediately clear, despite its functionality. Perhaps we should consider little bit refactoring lines 513-521 to clarify the logic?
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 agree, especially because this is where the three streams of input converge. I have tried to clarify in more procedural detail:
There are several ways of selecting no tools:
- Using
--config
and omittingtools:
from config kedro new --tools=none
- User prompts and using the default value or inputing none
- Config file entry
tools: "none"
The condition at line 514, is still triggered by cases 3 and 4 (as "none" != None
), which will result in extra_context['tools'] = []
.
Line 519 then checks against this, only converting the list to a string to be stored if the list is not empty.
All cases 1-4 will skip to line 521 as by this point in the execution extra_context['tools']
will either be None
or an empty list []
, unifying the stored result as ['None']
I am unsure we could refactor only these lines without affecting the rest of the file, did you have any suggestions?
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.
@AhdraMeraliQB , I agree, it's really a lot of ways to select tools.
And I also agree that we should split some code before lines 513-521. I think we should clarify steps:
- Finalise the value of extra_context["tools"] as a list of numbers, or [] by default anyway. It should be done before lines 513.
- Clarify that now in lines 513 we want to convert that list of numbers to list of Readable names and [] (empty list) will be converted to a ["None"]
So split the collection and merging step from conversion step.
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 would be in favour of doing the refactoring in this PR, because the change introduced here makes the code even harder to follow. I understand why it's needed now, but it's hard to grasp it just from reading the code.
Signed-off-by: Ahdra Merali <[email protected]>
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.
This mostly looks good to me, just one comment about the extra if statement
kedro/framework/cli/starters.py
Outdated
@@ -516,9 +516,10 @@ def _get_extra_context( # noqa: PLR0913 | |||
NUMBER_TO_TOOLS_NAME[tool] | |||
for tool in _parse_tools_input(tools) # type: ignore | |||
] | |||
extra_context["tools"] = str(extra_context["tools"]) | |||
if extra_context.get("tools"): |
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.
What does this additional check do on top of this check above?
tools = extra_context.get("tools")
if tools:
Unifying
|
Signed-off-by: Ahdra Merali <[email protected]>
@noklam re-requested review due to logic refactor |
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.
Thanks @AhdraMeraliQB, this is much clearer now! 👍
This reverts commit 5239190.
Signed-off-by: Ahdra Merali <[email protected]>
Here are the changes between the refactor and the string wrapper, had to revert for testing purposes, but no logic changes (just linting) |
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.
@AhdraMeraliQB thank you, amazing work!
@AhdraMeraliQB , looks like now we receive a message "You have selected no project tools" even if using just a starter, where no tools needed. |
Interesting..., Probably another consequence of unifying None and "none" |
I think the main reason is that in general internal logic is little unclear, we don't properly internally split situation where we it just starters flow or it's a general flow, it all mixed. Also 3 possible inputs together with providing default values are also mixed. With that it's complicated to manage it properly. |
Description
I noticed after #3426 supplying
--tools=none
to kedro new no longer resulted in the message "You have selected no project tools". This PR fixes this, and adds automated tests for this test case, and for providing tools through the config file, both of which were previously untested.Development notes
Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file