Skip to content

Fixing #46 by adding an optional argument to disable riase_for_status #55

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sferich888
Copy link

What kind of change does this PR introduce?

Fixes #46

What is the current behavior?

Currently riase_for_status will mask/eat errors from graphql servers that can respond explaining the error.
This allows users to disable that functionality and have the json of the response returned; which would include the error.

What is the new behavior?

Default behavior remains the same; however an optional argument is now available that lets user disable the use of riase_for_status to catch errors.

Does this PR introduce a breaking change?

No; not that I am aware of.

Other information

@sferich888
Copy link
Author

@DaleSeo can I get one of the repo maintainers to review

Copy link
Contributor

@xkludge xkludge left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, could you by chance add a unit test that will cover the two scenarios of toggling this flag?

@@ -37,6 +37,7 @@ def execute(
variables: dict = None,
operation_name: str = None,
headers: dict = {},
riase_for_status: bool = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

By chance did you mean?

Suggested change
riase_for_status: bool = True,
raise_for_status: bool = True,

@@ -50,8 +51,10 @@ def execute(
headers={**self.headers, **headers},
**{**self.options, **kwargs},
)

if riase_for_status:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if riase_for_status:
if raise_for_status:

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.

Allow status codes, other than 200
2 participants