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

Update to faraday 1.0 #196

Merged
merged 8 commits into from
Jan 11, 2020
Merged

Conversation

BobbyMcWho
Copy link
Contributor

@BobbyMcWho BobbyMcWho commented Sep 28, 2019

Faraday 0.16.0 compatibility, drops support for rubies < 2.3.0, jruby and rubinius

@@ -102,6 +102,8 @@ def update_env(env, request_body, response)
redirect_to_url = safe_escape(response['location'] || '')
env[:url] += redirect_to_url

ENV_TO_CLEAR.each { |key| env.delete key }

Copy link
Contributor Author

@BobbyMcWho BobbyMcWho Sep 28, 2019

Choose a reason for hiding this comment

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

This needs to happen before we access env[:body] otherwise it returns the response_body and not the request_body https://github.com/lostisland/faraday/blob/v1.0-rc1/UPGRADING.md

@BobbyMcWho BobbyMcWho marked this pull request as ready for review September 28, 2019 19:23
Faraday 0.16.0 changed it so that env[:body] aliases
to either env[:request_body] or env[:response_body]
https://github.com/lostisland/faraday/blob/v1.0-rc1/UPGRADING.md
Drop support for jruby and rbinx

Don't exclude versions
@BobbyMcWho
Copy link
Contributor Author

@olleolleolle are you maintainer over here too?

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

This change makes a new CI matrix:

  • 2.3
  • 2.4
  • 2.5.0
  • 2.6.3

This is a consequence of the forward movement in Faraday 0.16.x.

README.md Outdated Show resolved Hide resolved
@@ -15,4 +15,3 @@ class Response
autoload :ParseYaml, 'faraday_middleware/response/parse_yaml'
end
end

Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't need to change, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was my editor automatically removing a duplicate newline. I don't have access to my machine today if you think it needs reverted

Co-Authored-By: Olle Jonsson <olle.jonsson@gmail.com>
iMacTia
iMacTia previously approved these changes Oct 2, 2019
Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Just a small nit on the travis matrix from me, good to go anyway

.travis.yml Outdated
gemfile:
- Gemfile
- Gemfile-0.9.rb
gemfile: Gemfile
Copy link
Member

Choose a reason for hiding this comment

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

This should be useless as it's the default value for gemfile

Copy link
Member

Choose a reason for hiding this comment

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

Cool, even easier to read!

@iMacTia iMacTia dismissed their stale review October 7, 2019 15:58

Dismissing as this PR should not be required anymore now that v0.16 has been rolled back

@iMacTia
Copy link
Member

iMacTia commented Oct 7, 2019

Parking this for now as we've released Faraday v0.17.0 which rolled back to v0.15.4

@BobbyMcWho
Copy link
Contributor Author

Yeah, this will likely target a 1.0 release of faraday_middleware instead.

spec.add_dependency 'faraday', ['>= 0.7.4', '< 1.0']
spec.add_dependency 'faraday', '~> 0.16.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

faraday 0.17.x and 1.x also use the new namespace.

Technically, 0.17.0 and 0.17.3 i think use the top-level namespace, while 0.17.1 and 0.17.2 provided a shim for the Errors module…

@iMacTia iMacTia changed the title Update to faraday 0.16.0 Update to faraday 1.0 Jan 10, 2020
@iMacTia
Copy link
Member

iMacTia commented Jan 10, 2020

Thanks for the work @BobbyMcWho! Now that Faraday 1.0 has been released we can finally get this merged 🎉

@iMacTia
Copy link
Member

iMacTia commented Jan 10, 2020

Just noticed the pessimistic operator, I've relaxed that to allow v1.0 as that's the version we want to target.
Breaking changes in v0.16.0 has been rolled-back in v0.17 so there's no need for these changes anymore prior to v1.0

@iMacTia iMacTia merged commit e169ab2 into lostisland:master Jan 11, 2020
This was referenced Jan 11, 2020
@d-m-u d-m-u mentioned this pull request Feb 4, 2020
21 tasks
composerinteralia added a commit to composerinteralia/octokit.rb that referenced this pull request Jun 20, 2024
The faraday_middleware this is based on made the same change in lostisland/faraday_middleware#196. On Faraday >= 1 referring to `env[:body]` when the status is set will refer to the response body rather than the request body.
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.

None yet

4 participants