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

Fixes #152: Set script exit code based on request success #153

Merged
merged 2 commits into from
Apr 24, 2023
Merged

Fixes #152: Set script exit code based on request success #153

merged 2 commits into from
Apr 24, 2023

Conversation

stephenmuss
Copy link
Contributor

This small change fixes #152 and ensures that the traceback from response.raise_for_status() is not included in the program's output.

Although the property response.ok uses raise_for_status() under the hood, the raised exception HTTPError is caught and a boolean value is returned.

Using the boolean value from ok we can determine whether to set a successful or unsuccessful exit code.

@okigan
Copy link
Owner

okigan commented Apr 23, 2023

Thanks for the PR!, it looks like I need to fix up the GitHub actions to validate the PR (so far, it's resisting) - please stand by :D.

@okigan
Copy link
Owner

okigan commented Apr 24, 2023

@stephenmuss please rebase your PR (it would be nice if GH had that feature)

@stephenmuss
Copy link
Contributor Author

@okigan have rebased

@okigan
Copy link
Owner

okigan commented Apr 24, 2023

You also need to update the unit test (search TestInnerMainMethod), should be straightforward.

To test locally, you can run tox or if you have docker installed, you can run act to mimic GitHub action checks.

@stephenmuss
Copy link
Contributor Author

@okigan I've updated the test to check for exit code and tests are passing for me locally. Thanks.

@okigan okigan merged commit 4ecf872 into okigan:master Apr 24, 2023
@stephenmuss stephenmuss deleted the catch-exception branch April 24, 2023 05:10
@stephenmuss
Copy link
Contributor Author

Thanks @okigan!

@okigan
Copy link
Owner

okigan commented Apr 24, 2023

new build is now published on PyPI, dockerhub, and GitHub Container Registry.
Homebrew will probably self-update over the next 24 hours.

Thank you for working through this!

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.

raise_for_request should not be used for non-successful response
2 participants