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

Report all errors as messages not as Exceptions #4914

Merged

Conversation

arielshaqed
Copy link
Contributor

This improves the UX and makes thing more scriptable and more UN*X-like.

Tested by manually running these failing commands:

❯ ./lakefs_export.py --branch asdf --commit_id sdf abc def
Cannot set both branch and commit

❯ ./lakefs_export.py abc def
Must set one of branch, commit

❯ ./lakefs_export.py --prev_commit_id asdf --commit_id sdf abc def
Cannot export diff between two commits.

Fixes #4912.

This improves the UX and makes thing more scriptable and more UN*X-like.

Tested by manually running these failing commands:
```sh
❯ ./lakefs_export.py --branch asdf --commit_id sdf abc def
Cannot set both branch and commit

❯ ./lakefs_export.py abc def
Must set one of branch, commit

❯ ./lakefs_export.py --prev_commit_id asdf --commit_id sdf abc def
Cannot export diff between two commits.
```

Fixes #4912.
@arielshaqed arielshaqed requested a review from johnnyaug January 1, 2023 11:42
Comment on lines 86 to 88
error("Cannot set both branch and commit_id")
else: # not has_branch and not has_commit
error("Must set one of branch, commit_id")
Copy link
Contributor

Choose a reason for hiding this comment

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

strange indentation here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Turns out we never flake8 this. Opening a new bug to do that, but for now fixed everything flake8 had to complain about. So as punishment PTAL :-/

@arielshaqed arielshaqed added area/tools Improvements or additions to tooling and scripting bug Something isn't working include-changelog PR description should be included in next release changelog pr/merge-if-approved Reviewer: please feel free to merge if no major comments labels Jan 1, 2023
Now `flake8 .` passes :-)
@arielshaqed arielshaqed requested a review from johnnyaug January 1, 2023 12:11
Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

LGTM

@arielshaqed
Copy link
Contributor Author

Thanks muchly!

@arielshaqed arielshaqed merged commit 4992d91 into master Jan 1, 2023
@arielshaqed arielshaqed deleted the bug/4912-lakefs_export-report-errors-not-exceptions branch January 1, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tools Improvements or additions to tooling and scripting bug Something isn't working include-changelog PR description should be included in next release changelog pr/merge-if-approved Reviewer: please feel free to merge if no major comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lakefs_export: Report errors as messages not exceptions
2 participants