Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MVP for "add-ons" flow within kedro new CLI command #2987
MVP for "add-ons" flow within kedro new CLI command #2987
Changes from 46 commits
3726ed7
592afb5
dd27d67
ad127d8
868816c
52e99dd
5108223
6cc090c
a0fe4a2
4fb0671
c1a1f7d
ba01bb2
0f92782
bc0c172
52ddf73
ded026c
f216847
ee668fd
a500a7a
b88f01c
fa379a0
f39ebad
4a4164d
ea40e0e
d56cc31
6208edb
66c9f36
58f211f
119465d
5434858
225f26a
2f866f7
06509fd
e73fe7e
bb4e358
1a52a3c
51d0c12
39761d0
da14a74
0160bbe
8609597
603fc93
aae48cb
ca6297a
4a9cdcb
3711567
e037441
08567d3
7fca19e
47d935e
e3c7de5
fd0dabd
f8b8920
57cf29c
951015d
d280307
8a51b26
abd467f
47d063f
488eca9
c5e3050
472a57c
e0e5077
e89ed0c
406393d
36d72dc
006555e
49b72b0
e4138d3
a3403de
3579326
2ad5078
608af72
78c89e4
01ad5c0
574cf01
902b858
c819148
618d4c0
1b9c23f
566462c
b5439d0
a8c8f86
b679b6a
90aad98
4314f23
e3800ec
20a8e61
c31a9f4
f7518a1
c292d0d
1332139
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 error here is very similar to the one above in
cli/starters.py
and I can see it gets triggered twice with a full stacktrace (see below). I don't think we should be showing the stacktrace at all and one message should be enough.It's so small because I couldn't otherwise screenshot it:
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.
Addressed in 472a57c - I tried isolating the ValueError from the stack trace to no avail (both
with_traceback(None)
andsys.tracebacklimit = 0
were unsuccessful, as well as several other approaches), I settled on just using sys.exit(1). Let me know your thoughts on this approach.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.
Nit: the formatting looks a bit weird for this doc string. Could you add some tabs to make it easier to read what are the arguments and what the description?
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.
Noticed this is used by the e2e test "Activate_nbstripout target in new project", which is not related to documentation, and so
nbstripout
should be present in the default bare-bones requirements fileThere 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
activate_nbstripout
command will be removed in0.19.0
, so I'd suggest just removing it here already if it doesn't interfere with tests.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.
Removing it causes the e2e test to fail, unfortunately
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.
This can now be removed because the commands + tests were removed by Nok in #3065