Add `add_ons` to `pyproject.toml` in spaceflight starters

kedro-org / kedro-starters

Templates for your Kedro projects.

Apache License 2.0

64 stars 59 forks source link

Add `add_ons` to `pyproject.toml` in spaceflight starters #175

Closed merelcht closed 11 months ago

merelcht commented 11 months ago

Motivation and Context

While working on https://github.com/kedro-org/kedro/issues/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

merelcht commented 11 months ago

@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 commented 11 months ago

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.

merelcht commented 11 months ago

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.

SajidAlamQB commented 11 months ago

With this change this is the difference in CLI flow:

Before:

After:

merelcht commented 11 months ago

With this change this is the difference in CLI flow:

Before:

After:

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.