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 to pyproject.toml in spaceflight starters #175

Merged
merged 10 commits into from
Nov 1, 2023

Conversation

merelcht
Copy link
Member

@merelcht merelcht commented Oct 31, 2023

Motivation and Context

While working on kedro-org/kedro#3220 I noticed the spaceflight starters didn't have the add_ons field in the pyproject.toml yet.

I only added it to the spaceflights starters, because those are the only ones that will be used in the new project creation flow.

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Assigned myself to the PR
  • Added tests to cover my changes

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht merelcht self-assigned this Oct 31, 2023
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

thank you! Good catch

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
@merelcht
Copy link
Member Author

@SajidAlamQB and @noklam I re-requested your reviews to make you aware I also had to update the default value from "none" to "['none']" to be parsed correctly. Does that look right to you? Or do we want to change this so "none" would also be accepted?

@noklam
Copy link
Contributor

noklam commented Nov 1, 2023

I think none should work as I expected this match the user prompt. How should I test this change? Note that this also doesn't match with https://github.com/kedro-org/kedro/blob/fd2151336afb5bf6ed792b336a07a7107e2e6703/kedro/framework/cli/starters.py#L219-L225C38 which return an []. We need to be clear hear should it return "none", ["none"] or []. I think either [] or ["none"] is fine.

spaceflights-pandas-viz/cookiecutter.json Outdated Show resolved Hide resolved
spaceflights-pandas/cookiecutter.json Outdated Show resolved Hide resolved
spaceflights-pyspark-viz/cookiecutter.json Outdated Show resolved Hide resolved
spaceflights-pyspark/cookiecutter.json Outdated Show resolved Hide resolved
@merelcht
Copy link
Member Author

merelcht commented Nov 1, 2023

I think none should work as I expected this match the user prompt. How should I test this change? Note that this also doesn't match with https://github.com/kedro-org/kedro/blob/fd2151336afb5bf6ed792b336a07a7107e2e6703/kedro/framework/cli/starters.py#L219-L225C38 which return an []. We need to be clear hear should it return "none", ["none"] or []. I think either [] or ["none"] is fine.

Actually none doesn't match the user prompt. Any user prompts via kedro new or kedro new --addons=none results in add_ons = [] in the pyproject.toml. However, "none" in cookiecutter.json will result in add_ons = none, which is invalid syntax. I've changed the default in cookiecutter.json to "[]" instead of "none", because that results in add_ons = [] which is the same as the CLI flow.

….toml

Co-authored-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com>
@SajidAlamQB
Copy link
Contributor

SajidAlamQB commented Nov 1, 2023

With this change this is the difference in CLI flow:

Before:

image

After:

image

@merelcht
Copy link
Member Author

merelcht commented Nov 1, 2023

With this change this is the difference in CLI flow:

Before:

image

After:

image

Ooohh good catch!! We definitely don't want that. I'll revert the default change, and just keep the change to pyproject.toml to reflect what you did on the viz add on PR.

@merelcht merelcht enabled auto-merge (squash) November 1, 2023 13:59
@merelcht merelcht merged commit 056ae42 into main Nov 1, 2023
17 of 18 checks passed
@merelcht merelcht deleted the add-ons-in-pyproject-toml branch November 1, 2023 14:17
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.

3 participants