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

Unhide cookiecutter errors in kedro new #3693

Merged
merged 10 commits into from
Mar 14, 2024

Conversation

ElenaKhaustova
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova commented Mar 11, 2024

Description

Close #2564

Development notes

  • Unhide cookiecutter errors in kedro new, so when --verbose flag is not provided short cookiecutter error is in the output; with --verbose flag full cookiecutter error is in the output;
  • Added unit test to check if original cookiecutter exception is present in the output if no --verbose flag is provided;
  • Updated the test_run_with_invalid_config test that was changing the behaviour of the added test by modifying the global object KedroCliError. The reason of it is calling _validate_config_file that sets KedroCliError.VERBOSE_EXISTS=False.

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: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
@ElenaKhaustova ElenaKhaustova force-pushed the feature/2564-unhide-cookiecutter-errors branch from 22b4cc9 to 8f82253 Compare March 11, 2024 13:51
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
def test_cookiecutter_exception_if_no_verbose(self, fake_kedro_cli):
"""Check if original cookiecutter exception present in the output if no verbose
flag provided."""
KedroCliError.VERBOSE_EXISTS = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean to reset the side-effect from the other test? If so I think we make sure this happen after that test so when we add new test in the future or reordering tests won't fail again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will move it to the end of the that test.

@ElenaKhaustova ElenaKhaustova changed the title Feature/2564 unhide cookiecutter errors Unhide cookiecutter errors in kedro new Mar 12, 2024
@ElenaKhaustova ElenaKhaustova marked this pull request as ready for review March 12, 2024 14:53
@datajoely
Copy link
Contributor

🥳

@ElenaKhaustova ElenaKhaustova self-assigned this Mar 13, 2024
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.

I left one question, but otherwise looks good! 👍

Congrats on your first PR 🥳

@@ -1,6 +1,7 @@
# Upcoming Release 0.19.4

## Major features and improvements
* Cookiecutter errors are shown in short format without the `--verbose` flag.
Copy link
Member

Choose a reason for hiding this comment

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

Are the error now shown partly for all CLI commands or just kedro new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now they're shown in short format for all commands but just for cookiecutter errors. So instead of Error: Failed to generate project when running cookiecutter. we will also get one line with a real error.

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.

🔶Congrats on your first PR🎉

@ElenaKhaustova ElenaKhaustova enabled auto-merge (squash) March 14, 2024 12:16
@ElenaKhaustova ElenaKhaustova merged commit 9efca0e into main Mar 14, 2024
41 checks passed
@ElenaKhaustova ElenaKhaustova deleted the feature/2564-unhide-cookiecutter-errors branch March 14, 2024 12:37
@astrojuanlu
Copy link
Member

This works beautifully, thanks @ElenaKhaustova !

Screenshot 2024-03-18 at 20 55 22

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.

Unhide cookiecutter errors in kedro new
5 participants