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

Add add_ons metadata to pyproject.toml for project creation #3188

Merged
merged 15 commits into from
Oct 24, 2023

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Oct 17, 2023

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

Development notes

I added more comments in the changed files. It's a bit tricky to get everything works

Overall there are few changes and refactoring. There are few benefit of using names over number:

  • Easy to read from telemetry
  • Flexible to re-order/change the number. This can be seen from the PR as I remove more code than writing new code.
  1. Move the parsing logic back to starter, cookiecutter will store the parsed list i.e ["Linting","Documentation"]

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 <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
Copy link
Contributor Author

noklam commented Oct 17, 2023

It's functional already, the question is whether we want to keep the options [1,2,3] or the name i.e. ["Linting","Documentation"]. I also have some question around the project creation flow since this is my first ticket related to this theme.

@noklam noklam linked an issue Oct 17, 2023 that may be closed by this pull request
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as ready for review October 20, 2023 16:27
@noklam noklam requested a review from merelcht as a code owner October 20, 2023 16:27
@noklam noklam requested review from SajidAlamQB and merelcht and removed request for merelcht October 20, 2023 16:27
Comment on lines 94 to 100
ADD_ONS_DICT = {
"1": "Linting",
"2": "Testing",
"3": "Custom Logging",
"4": "Documentation",
"5": "Data structure",
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This need to be used multiple place, probably tests as well.

@@ -161,6 +166,50 @@ def _starter_spec_to_dict(
return format_dict


def _parse_add_ons_input(add_ons_str: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move from the hooks - parsing logic should be in cli.py instead of post_hook

docs_path = current_dir / "docs"
if docs_path.exists():
shutil.rmtree(str(docs_path))
else:
with open(pyproject_file_path, 'a') as file:
file.write(docs_pyproject_requirements)

if "5" not in selected_add_ons_list: # If Data Structure not selected
if "Data Structure" not in selected_add_ons_list:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this name finalised already? I find this a bit awkward and doesn't match the terminology in our docs? cc @amandakys

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On #3216 she commented it should be "Data Folder"

@noklam noklam marked this pull request as draft October 20, 2023 16:34
Comment on lines 404 to 409
add_ons = config.get("add_ons")
if add_ons:
config["add_ons"] = [
ADD_ONS_DICT[add_on] for add_on in _parse_add_ons_input(add_ons)
]
config["add_ons"] = str(config["add_ons"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the cookiecutter.add_ons to the actual name instead of nunber.

@noklam
Copy link
Contributor Author

noklam commented Oct 20, 2023

I will mark as ready for review when the tests are fixed.

Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
h
Merge branch 'noklam/ci-add-on' of https://github.com/kedro-org/kedro into noklam/ci-add-on

Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
"2": "Testing",
"3": "Custom Logging",
"4": "Documentation",
"5": "Data structure",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data structure -> Data Structure

Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as ready for review October 20, 2023 16:58
@noklam noklam requested a review from ankatiyar October 23, 2023 13:02
Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @noklam, overall, the refactoring looks great and cleaner. I've tested it manualy and its working as expected. 🌟🌟

kedro/framework/cli/starters.py Show resolved Hide resolved
@@ -353,11 +402,20 @@ def _make_cookiecutter_args(
"""
config.setdefault("kedro_version", version)

# Map the selected add on lists to readable name
add_ons:str = config.get("add_ons")
if add_ons:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add_ons ever be null?

@noklam noklam removed the request for review from merelcht October 23, 2023 16:07
@noklam
Copy link
Contributor Author

noklam commented Oct 23, 2023

Removed @merelcht from reviewer list since she is sick. Would be still great to review if this isn't merged yet but I have asked @ankatiyar for review because this PR will block a few other PR so it best to get merged soon.

Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this manually. Also good work on refactoring :D

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 👍

This might have some impact on #3081, so would be good to coordinate with @lrcouto when it comes to merging.

RELEASE.md Outdated
@@ -5,6 +5,7 @@
* Introduced add-ons to the `kedro new` CLI flow.

## Bug fixes and other changes
- Added a new field `add-ons` to `pyproject.toml` when a project is created.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit 🙈

Suggested change
- Added a new field `add-ons` to `pyproject.toml` when a project is created.
* Added a new field `add-ons` to `pyproject.toml` when a project is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 I keep forgetting this since - is easier to type ...

docs_path = current_dir / "docs"
if docs_path.exists():
shutil.rmtree(str(docs_path))
else:
with open(pyproject_file_path, 'a') as file:
file.write(docs_pyproject_requirements)

if "5" not in selected_add_ons_list: # If Data Structure not selected
if "Data Structure" not in selected_add_ons_list:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On #3216 she commented it should be "Data Folder"

noklam and others added 3 commits October 24, 2023 13:57
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam noklam merged commit 00f035a into develop Oct 24, 2023
52 of 58 checks passed
@noklam noklam deleted the noklam/ci-add-on branch October 24, 2023 19:09
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.

Add selected add-ons to the created project's metadata
4 participants