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

Revert raise-on-non-2xx response behavior #54

Merged
merged 4 commits into from
Jan 20, 2022
Merged

Conversation

ltk
Copy link
Member

@ltk ltk commented Jan 20, 2022

3b02b32 aimed to fix the originally intended behavior of raising custom errors for most non-2xx responses. This however resulted in a fairly significant change to the actual interface of the 0.2.0 release (in which this behavior seems to have not been working as intended), so we're reverting it.

We may restore the error raising behavior behind a feature flag in an upcoming release.

ltk added 3 commits January 20, 2022 11:34
The 0.2.0 release did not include this behavior, so we're reverting it.
We may choose to add this behavior back in under a configurable flag
in the near future.
end
end
end
# describe 'errors' do

Choose a reason for hiding this comment

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

Let's change the tests to describe the actual behaviour, instead of commenting them out. Since this behaviour is already published and used, we can't safely change it now, so I'd say the tests should describe the actual instead of desired behaviour.

I'd probably treat removing RaiseError as a breaking change, so this would go in oktakit 1.0 or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I agree that removing the raising feature completely would be a major version bump. This next minor version will just be in a weird hybrid state that should work for clients regardless of their faraday version.

@ltk ltk merged commit 1c58db5 into master Jan 20, 2022
@ltk ltk deleted the ltk/revert-3b02b3227 branch January 20, 2022 19:12
@simonlevasseur
Copy link

simonlevasseur commented Apr 4, 2022

change to the actual interface of the 0.2.0 release

Could you expand on this? I don't see anything in the CHANGELOG or release notes that would indicate a change in interface?

Since this file exists, it seems clear that the intention was to raise an Oktakit-based error that could later be rescued but now this middleware isn't called at all. It's also worth noting that this change occurred when Faraday was updated and not based on a code change in this repo.

@Ginja Ginja mentioned this pull request Jul 12, 2023
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