search

minimul / qbo_api

Ruby JSON-only client for QuickBooks Online API v3. Built on top of the Faraday gem.
MIT License
85 stars 46 forks source link

Remove faraday middleware (try 2) #118

Closed mculp closed 1 year ago

mculp commented 1 year ago

Resubmitting @bf4's PR with the requested changes. We've been stuck on upgrading Faraday due to this issue for months.

Let me know if there's anything else that I need to change.

mculp commented 1 year ago

Is there no CI / automated checks for specs? :D

...

😬

mculp commented 1 year ago

ok, I think we're good:

............................................................................

Finished in 0.26466 seconds (files took 0.42267 seconds to load)
76 examples, 0 failures
mculp commented 1 year ago

Had to make a few more changes to get tests passing.

mculp commented 1 year ago

Here are the changes: https://github.com/bf4/qbo_api/compare/remove_faraday_middleware...mculp:qbo_api:remove_faraday_middleware

mculp commented 1 year ago

Ok, I think that covers the feedback. Let me know if you need anything else! Thanks!

minimul commented 1 year ago

Thanks @mculp and @bf4 !

minimul commented 1 year ago

Version 3.0.0 released.

bf4 commented 1 year ago

:dancer:

On Tue, Jan 3, 2023, 7:26 AM Christian Pelczarski @.***> wrote:

Thanks @mculp https://github.com/mculp and @bf4 https://github.com/bf4 !

— Reply to this email directly, view it on GitHub https://github.com/minimul/qbo_api/pull/118#issuecomment-1369711249, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABC4QQYKUZ5OAY3VUBKZMTWQQLHNANCNFSM6AAAAAATNYGIZA . You are receiving this because you were mentioned.Message ID: @.***>

bf4 commented 1 year ago

Great collaboration everyone! 🎩

we're now on Faraday 2.7.2 (from 1.10.2)

only 'gotcha' is that that the JSON faraday-middleware would parse a response with content-type: 'text/html' as JSON whereas the now used faraday response middleware checks the content type. the upshot is that request Accept: application/json with response Content-Type: text/html no longer raises a Faraday::ParsingError

minimul commented 1 year ago

Thanks @bf4 for first getting the ball rolling.

Regarding the 'gotcha' – should we make a new issue or just document it?

bf4 commented 1 year ago

@minimul I'm not sure. We could force the 'old' behavior by setting 

builder.response :json, content_type: /./, # always parse as JSON, even when is text/html, in order to raise a Faraday::ParsingError