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

Fixing help message and improving error handling #83

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

tommyg-cld
Copy link
Contributor

@tommyg-cld tommyg-cld commented Nov 7, 2023

Brief Summary of Changes

What does this PR address?

  • GitHub issue (Add reference - #XX)
  • Refactoring
  • New feature
  • Bug fix
  • Adds more tests

Are tests included?

  • Yes
  • No

Reviewer, please note:

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I ran the full test suite before pushing the changes and all the tests pass.

@@ -25,7 +24,7 @@
@option("-A", "--auto_paginate", is_flag=True, default=False,
help="Auto-paginate Admin API calls.")
@option("-F", "--force", is_flag=True,
help="Skip initial and auto-paginate confirmation.")
help="Skip initial confirmation.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change to Skip confirmation. or Skip user confirmation.. like, initial is kind of ambiguous, no context.
@carlevison, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@const-cloudinary 'Skip confirmation.' is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to Skip confirmation.

@@ -63,6 +62,9 @@ def regen_derived(trans_str, eager_notification_url,
api_name="admin",
auto_paginate=auto_paginate,
force=force, return_data=True)
if trans_details is False:
exit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe write an error message saying what went wrong? Just exiting can be very confusing. And it you run it in script/cron job - you have no idea what happened/ did not happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed I've updated this to return False instead

@const-cloudinary const-cloudinary merged commit bf14037 into master Nov 7, 2023
10 checks passed
@const-cloudinary const-cloudinary deleted the fix/help-message-and-error-handling branch November 7, 2023 14:05
@const-cloudinary
Copy link
Contributor

@tommyg-cld great PR!

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.

3 participants