Make project creation flow parse add-ons consistently #3265

noklam · 2023-11-03T03:48:52Z

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Address Point 2 in #3264, I found some opportunities to improve the code.

Development notes

There are many conversion between lint -> 1 -> Linting, sometimes they are not used consistently. This PR mainly address this and fixed some other minor thing, I have commented with inline PR comments. Cc @lrcouto @merelcht

Rename config -> extra_context to reflect what it does
Refactor the 3 way conversion between lint -> 1 ->Linting into constants.

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

Signed-off-by: Nok Chan <[email protected]>

…dictionary Signed-off-by: Nok Chan <[email protected]>

Signed-off-by: Nok Chan <[email protected]>

This reverts commit bf9e639. Signed-off-by: Nok Chan <[email protected]>

Signed-off-by: Nok Chan <[email protected]>

noklam · 2023-11-03T04:15:01Z

kedro/framework/cli/starters.py

@@ -322,15 +335,15 @@ def new(  # noqa: too-many-arguments
    shutil.rmtree(tmpdir, onerror=_remove_readonly)

    # Obtain config, either from a file or from interactive user prompts.
-    config = _get_config(
+    extra_context = _get_extra_context(


this is passed as extra_context to cookiecutter, so I think it's better just name it extra_context.

noklam · 2023-11-03T04:16:01Z

kedro/framework/cli/starters.py

@@ -488,7 +494,7 @@ def _select_prompts_to_display(
        del prompts_required["add_ons"]

    if project_name is not None:
-        if bool(re.match(r"^[\w -]{2,}$", project_name)) is False:
+        if not re.match(r"^[\w -]{2,}$", project_name):


the boolean expression is not needed. re.match either return an object or None

noklam · 2023-11-03T04:17:19Z

tests/framework/cli/test_starters.py

@@ -68,25 +69,6 @@ def _make_cli_prompt_input_without_name(
    return "\n".join([add_ons, repo_name, python_package])


-def _convert_addon_names_to_numbers(selected_addons: str):


this function is actually more explicit than the _get_addons_from_cli_input. It is only used in test originally, I've rename the original function and remove this from test.

Signed-off-by: Nok Chan <[email protected]>

merelcht

Thanks @noklam ! These improvements make it a lot easier to follow the code, especially the changes you made to have ADD_ONS_SHORTNAME_TO_NUMBER and NUMBER_TO_ADD_ONS_NAME dicts 😃

deepyaman

Minor suggestions

deepyaman · 2023-11-03T15:50:21Z

kedro/framework/cli/starters.py

+ADD_ONS_SHORTNAME_TO_NAME = {
+    short_name: NUMBER_TO_ADD_ONS_NAME[num]
+    for short_name, num in ADD_ONS_SHORTNAME_TO_NUMBER.items()
+}
+


Nit: This seems unnecessary? You can just use list(ADD_ONS_SHORTNAME_TO_NUMBER) below.

Suggested change

ADD_ONS_SHORTNAME_TO_NAME = {

short_name: NUMBER_TO_ADD_ONS_NAME[num]

for short_name, num in ADD_ONS_SHORTNAME_TO_NUMBER.items()

}

Yeah actually initially I was thinking I can skip the number and just convert from the short name to the longer one. It's a bit tricky and since there are other PR working on this I don't want to create too much merge conflict.

I took your suggestion and remove this extra dict for now, will revisit.

deepyaman · 2023-11-03T15:51:24Z

kedro/framework/cli/starters.py

+ADD_ONS_SHORTNAME_TO_NUMBER = {
+    "lint": "1",
+    "test": "2",
+    "log": "3",
+    "docs": "4",
+    "data": "5",
+    "pyspark": "6",
+}
+NUMBER_TO_ADD_ONS_NAME = {


Nit :)

Suggested change

ADD_ONS_SHORTNAME_TO_NUMBER = {

"lint": "1",

"test": "2",

"log": "3",

"docs": "4",

"data": "5",

"pyspark": "6",

}

NUMBER_TO_ADD_ONS_NAME = {

ADD_ON_SHORTNAME_TO_NUMBER = {

"lint": "1",

"test": "2",

"log": "3",

"docs": "4",

"data": "5",

"pyspark": "6",

}

NUMBER_TO_ADD_ON_NAME = {

deepyaman · 2023-11-03T15:53:19Z

kedro/framework/cli/starters.py

@@ -213,13 +226,13 @@ def _validate_range(start, end):

    def _validate_selection(add_ons: list[str]):
        for add_on in add_ons:
-            if int(add_on) < 1 or int(add_on) > len(ADD_ONS_DICT):
+            if int(add_on) < 1 or int(add_on) > len(NUMBER_TO_ADD_ONS_NAME):


Suggested change

if int(add_on) < 1 or int(add_on) > len(NUMBER_TO_ADD_ONS_NAME):

if add_on not in NUMBER_TO_ADD_ON_NAME:

?

good found!

deepyaman · 2023-11-03T15:53:52Z

kedro/framework/cli/starters.py

    prompts_required: dict,
    config_path: str,
    cookiecutter_context: OrderedDict,
    selected_addons: str,
    project_name: str,
 ) -> dict[str, str]:
-    """Generates a config dictionary to be used to generate cookiecutter args, based
+    """Generates a config dictionary that will be passed to cookiecutters as `extra_context`, based


Suggested change

"""Generates a config dictionary that will be passed to cookiecutters as `extra_context`, based

"""Generates a config dictionary that will be passed to cookiecutter as `extra_context`, based

deepyaman

Actually, let me approve with the change requests, so I don't end up blocking :)

lrcouto

I'd implement Deepyaman's suggestions, but other than that it looks nice! It's easier to read now, thank you!

SajidAlamQB

LGTM, thanks @noklam!

Signed-off-by: Nok Chan <[email protected]>

noklam added 5 commits November 3, 2023 03:19

refactor the mapping to a constant

c91b2a8

Signed-off-by: Nok Chan <[email protected]>

add a new mapping to convert short name to long name easily

62e838f

Signed-off-by: Nok Chan <[email protected]>

shortcut return if add_ons is None, refactor the logic with a helper …

3c6206b

…dictionary Signed-off-by: Nok Chan <[email protected]>

refactor variable name to reflect what it does

bf9e639

Signed-off-by: Nok Chan <[email protected]>

rename config to extra_context

ba57dfb

Signed-off-by: Nok Chan <[email protected]>

noklam changed the base branch from main to develop November 3, 2023 03:49

noklam added 8 commits November 3, 2023 03:50

more refactor

ad23189

Signed-off-by: Nok Chan <[email protected]>

Revert "refactor variable name to reflect what it does"

dc54eb9

This reverts commit bf9e639. Signed-off-by: Nok Chan <[email protected]>

refactor the list with the constants dict

9d4c668

Signed-off-by: Nok Chan <[email protected]>

remove unncessary expression

e6f1154

Signed-off-by: Nok Chan <[email protected]>

rename config -> extra_context

20a6204

Signed-off-by: Nok Chan <[email protected]>

_get_config -> _get_extra_context

14103a8

Signed-off-by: Nok Chan <[email protected]>

rename all config reference to extra_context

39789e6

Signed-off-by: Nok Chan <[email protected]>

fix lint

0a4bdbf

Signed-off-by: Nok Chan <[email protected]>

noklam changed the title ~~Fix inconsistent logic handling cookiecutter argus~~ Fix inconsistent logic handling cookiecutter args Nov 3, 2023

noklam commented Nov 3, 2023

View reviewed changes

rename to avoid overwriting loop variable - ruff suggestion

a9bfa65

Signed-off-by: Nok Chan <[email protected]>

noklam marked this pull request as ready for review November 3, 2023 04:22

noklam requested a review from merelcht as a code owner November 3, 2023 04:22

noklam requested review from DimedS and lrcouto November 3, 2023 04:22

noklam changed the title ~~Fix inconsistent logic handling cookiecutter args~~ A few improvement to make project creation flow parse addon consistently Nov 3, 2023

This was referenced Nov 3, 2023

Minor tech debts in the project creation flow #3264

Closed

[Parent] - Add-ons Project Creation flow #3216

Closed

merelcht approved these changes Nov 3, 2023

View reviewed changes

noklam requested a review from SajidAlamQB November 3, 2023 15:43

deepyaman changed the title ~~A few improvement to make project creation flow parse addon consistently~~ Make project creation flow parse add-ons consistently Nov 3, 2023

deepyaman requested changes Nov 3, 2023

View reviewed changes

deepyaman approved these changes Nov 3, 2023

View reviewed changes

lrcouto approved these changes Nov 3, 2023

View reviewed changes

SajidAlamQB approved these changes Nov 3, 2023

View reviewed changes

refactor based on reviews suggention

9bdc3b6

Signed-off-by: Nok Chan <[email protected]>

noklam enabled auto-merge (squash) November 3, 2023 16:31

noklam merged commit 2ed06f5 into develop Nov 3, 2023
28 of 29 checks passed

noklam deleted the fix/inconsistent_add_ons branch November 3, 2023 16:59

noklam mentioned this pull request Nov 6, 2023

Adjust add-on input validation #3263

Merged

7 tasks

noklam self-assigned this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make project creation flow parse add-ons consistently #3265

Make project creation flow parse add-ons consistently #3265

noklam commented Nov 3, 2023 •

edited

Loading

noklam Nov 3, 2023

noklam Nov 3, 2023

noklam Nov 3, 2023

merelcht left a comment

deepyaman left a comment

deepyaman Nov 3, 2023

noklam Nov 3, 2023

deepyaman Nov 3, 2023

deepyaman Nov 3, 2023

noklam Nov 3, 2023

deepyaman Nov 3, 2023

deepyaman left a comment

lrcouto left a comment

SajidAlamQB left a comment

		@@ -68,25 +69,6 @@ def _make_cli_prompt_input_without_name(
		return "\n".join([add_ons, repo_name, python_package])


		def _convert_addon_names_to_numbers(selected_addons: str):

	if int(add_on) < 1 or int(add_on) > len(NUMBER_TO_ADD_ONS_NAME):
	if add_on not in NUMBER_TO_ADD_ON_NAME:

	"""Generates a config dictionary that will be passed to cookiecutters as `extra_context`, based
	"""Generates a config dictionary that will be passed to cookiecutter as `extra_context`, based

Make project creation flow parse add-ons consistently #3265

Make project creation flow parse add-ons consistently #3265

Conversation

noklam commented Nov 3, 2023 • edited Loading

Description

Development notes

Developer Certificate of Origin

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

deepyaman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepyaman left a comment

Choose a reason for hiding this comment

lrcouto left a comment

Choose a reason for hiding this comment

SajidAlamQB left a comment

Choose a reason for hiding this comment

noklam commented Nov 3, 2023 •

edited

Loading