Add tests to all new project creation flows

[AhdraMeraliQB]

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

• Read the contributing guidelines
• Signed off each commit with a Developer Certificate of Origin (DCO)
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[AhdraMeraliQB]

Extra condition to catch:

• User prompts tools:"none"

[merelcht]

What does this additional check do on top of this check above?

 tools = extra_context.get("tools")
    if tools:

[AhdraMeraliQB]

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)

[DimedS]

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?

[AhdraMeraliQB]

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:

1. Using --config and omitting tools: from config
2. kedro new --tools=none
3. User prompts and using the default value or inputing none
4. 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?

[DimedS]

@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:

1. Finalise the value of extra_context["tools"] as a list of numbers, or [] by default anyway. It should be done before lines 513.
2. 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.

[merelcht]

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.

[merelcht]

This mostly looks good to me, just one comment about the extra if statement

[merelcht]

What does this additional check do on top of this check above?

 tools = extra_context.get("tools")
    if tools:

[AhdraMeraliQB]

Unifying extra_context["tools"]="none" and extra_context["tools"] = None requires the logic on starters to be updated, which has been done in this PR - note that tests on this branch will fail until that PR is merged. Appended below is the PR description:

When a starter is selected, any removal of tools should not be applied to the template project. To prevent this, the post project generation hook would check that tools have been specified to gauge if this was a starter selection or example pipeline selection (where the latter would continue with the removal process).

However, this overlooked the case of no tools selected and an example pipeline being requested - the resulting project should have the unwanted tools stripped of it, but instead the project template was treated like a starter and no modifications were made. This is addressed by testing for an example-pipeline instead of a tools selection - this check is more robust, particularly because example-pipeline cannot be true when a starter flag is used.

[AhdraMeraliQB]

@noklam re-requested review due to logic refactor

[merelcht]

Thanks @AhdraMeraliQB, this is much clearer now! 👍

[AhdraMeraliQB]

Here are the changes between the refactor and the string wrapper, had to revert for testing purposes, but no logic changes (just linting)

[DimedS]

@AhdraMeraliQB thank you, amazing work!

[DimedS]

@AhdraMeraliQB , looks like now we receive a message "You have selected no project tools" even if using just a starter, where no tools needed.

[AhdraMeraliQB]

Interesting...,

Probably another consequence of unifying None and "none"

[DimedS]

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.