Skip to content

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?

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,

**{**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