Change to short names in config

[DimedS]

Description

This PR addresses Issue 4 from #3335, implementing the following changes:

1. Altered the tools format derived from the config file to use short names instead of numbers.
2. Updated both unit and E2E tests to reflect these changes. Combined test_tools_flag_none() with test_valid_tools_flag() as the only difference between them is a single line.
3. Updated or added missing comments to ensure they are current.
4. Consolidated the functions for fetching, validating, and parsing the configuration from a file into a single function.
5. Modified the _convert_tool_names_to_numbers() function to return a list directly instead of string, eliminating the need for additional processing. The _parse_tools_input() function was utilized for this purpose.
6. Restructured the _get_extra_context() function for enhanced clarity. It's divided into distinct sections, with a specific section for default values.
7. Improved the _validate_range(start, end) function, which checks the range interval of tools. Previously, entering an excessively large end value would cause the prompt to freeze. An additional check for the upper limit of the range has been introduced to prevent this issue.

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

[ankatiyar]

Manually tested, works well! Thanks @DimedS!

[AhdraMeraliQB]

Could you update the tools docs pages too within this PR?

• https://docs.kedro.org/en/stable/starters/new_project_tools.html#specify-tools-configuration-using-kedro-new-config
• https://docs.kedro.org/en/stable/starters/index.html#:~:text=from%20comma%2Dseparated-,values%2C%20ranges%2C,-or%20%E2%80%98all/none

Additionally don't forget to put these changes in RELEASE.md with a note about them breaking tools config

[merelcht]

The code is starting to look a lot better with these refactorings, but I think there's still a bit more we can do. I've left comments with some suggestions.

[DimedS]

Could you update the tools docs pages too within this PR?

• https://docs.kedro.org/en/stable/starters/new_project_tools.html#specify-tools-configuration-using-kedro-new-config
• https://docs.kedro.org/en/stable/starters/index.html#:~:text=from%20comma%2Dseparated-,values%2C%20ranges%2C,-or%20%E2%80%98all/none

Additionally don't forget to put these changes in RELEASE.md with a note about them breaking tools config

thank you for the links @AhdraMeraliQB , fixed

[DimedS]

The code is starting to look a lot better with these refactorings, but I think there's still a bit more we can do. I've left comments with some suggestions.

Thank you, @merelcht , for your valuable suggestions. I've implemented them and they indeed make a lot of sense.

Could you help me understand the purpose of the test_fetch_config_from_user_prompts_with_context() function? I've temporarily removed it as I'm unsure how to modify it after updating the fetch_config_from_user_prompts() function. Your guidance on this would be greatly appreciated.

[merelcht]

Thank you, @merelcht , for your valuable suggestions. I've implemented them and they indeed make a lot of sense.

Could you help me understand the purpose of the test_fetch_config_from_user_prompts_with_context() function? I've temporarily removed it as I'm unsure how to modify it after updating the fetch_config_from_user_prompts() function. Your guidance on this would be greatly appreciated.

I added that test to cover these lines:

kedro/kedro/framework/cli/starters.py

Lines 694 to 695 in 34e66c3

	if not cookiecutter_context:
	raise Exception("No cookiecutter context available.")

Test coverage seems to be fine though, so I guess it might be tested in another way now and test_fetch_config_from_user_prompts_with_context isn't needed anymore.

[merelcht]

str(["None"]) still seems like a bit of an odd value to me.. We don't have to change that in this PR, but I think it would be good to discuss.

[DimedS]

I agree with that

[AhdraMeraliQB]

Idea, just noting this here but not adding to the scope of the PR - could we save it as an unwrapped list ["None"] instead?

Context:

The reason we pass through a list wrapped in a string is because cookiecutter interprets a list as a provision of options, from which it selects one to save. To preserve all the chosen tools, we wrap the list in a string. Though the option for no tools is processed separately, pyproject.toml has no knowledge of what is passed through and so we cannot pass through just the string "None" (which will be unwrapped).

However, none tools cannot be selected with any other tools, and so passing through just a list shouldn't have any adverse effects; cookiecutter would pick the only option from the list - "None" - and pass that through to pyproject.toml.

Would we run into the same problem with unwrapping strings? If so we could always pass through [["None"]] but it doesn't seem much better than str(["None"]) 🤔

Additionally, for telemetry, should we be consistent with what we store (a list for several tools, but this would save a string if no tools are selected)?

Alternatively, I believe using dictionaries to pass a collection of objects through cookiecutter is possible, but it would require significant changes to pyproject.toml - I'm unsure we'd be able to preserve the list type as we'd have to somehow process these values within the cookiecutter execution.

Any opinions on this? CC: @DimedS @merelcht

[DimedS]

@AhdraMeraliQB , and why we need a string of a list? Why just a string is not enough, like:
tools = "Linting, Testing, ..." or tools = "None"

[AhdraMeraliQB]

@DimedS
If I remember correctly, the reason is tied to telemetry - it's easier (and more intuitive) to query tools data when stored as a list than as a string. We did originally pass this through as a string (xref), but was suggested to be changed as part of #3264 and in #3333

The fix in #3426 introduced using str["None"] to workaround cookiecutter limitations (see Nok's comment) and the rest is history 😅

[AhdraMeraliQB]

Leaving my comments on the implementation here, they affect some of the choices made in test_starters.py so I'll wait to resolve these before continuing the review of that file

[AhdraMeraliQB]

This looks amazing!! Thank you for all the changes - only one significant question (re: the error messages) from me 😄

[AhdraMeraliQB]

Fantastic work @DimedS!! Thanks for making all the changes, this will make the file so much easier to read 🌟

The PR has changed quite a bit since the first approval, I've re-requested @ankatiyar to take a new look at this before we give it the final send-off 🚀

[ankatiyar]

Thanks @DimedS for the fix as well as a refactoring! 👍🏾

[DimedS]

Thank you for your contribution @AhdraMeraliQB , @ankatiyar , @merelcht !