-
Notifications
You must be signed in to change notification settings - Fork 906
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
Check if starter is used before printing tools message #3484
Conversation
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
af02d90
to
af8e236
Compare
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
The kedro/kedro/framework/cli/starters.py Lines 250 to 261 in 30ce26c
into a separate function that validates the chosen arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks good to me 👍 I left some comments to tidy the code some more.
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments from my side, but in general this is looking good!
"Cannot use the --starter flag with the --example and/or --tools flag." | ||
) | ||
|
||
flag_inputs = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why create a dict and not just directly pass the arguments to _validate_flag_inputs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's in general a good idea - to put all CLI inputs to one dictionary, because currently there are a lot of them, but it should be done not only for one _validate_flag_inputs() function, but to the whole subsequent code. Better to do it little bit later - it will be a huge change, otherwise I cannot merge my current PR #3499 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, the initial reason was due to how many inputs there are, and for future implementation/refactor as there is no guarantee that we won't need to pass these through later. I couldn't find any strong support for either convention, however, so I am mostly ambivalent. Though whatever we do choose, we should be sure to implement consistently.
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
…dro into fix/clean-up-starters-logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @AhdraMeraliQB , awesome job! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Description
As pointed out by @DimedS: xref
We don't allow users to select tools when using starters, so the message "you have selected no project tools" should not be displayed if a starter is used. This check was previously implicit, relying on the different values of tools to differentiate (previously
'[]'
and['None']
). These have since been unified, and as such, an explicit condition is required to ensure the message isn't printed when a starter is used.Development notes
There are several alternatives for implementing this:
starter_alias
<- current approachstarter_alias
through to_create_project
as an argumentstarter_alias
through as part of theextra_config
to be accessed in_create_project
<- original approachstarters.py
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
RELEASE.md
file