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

Fix 1.9 syntax error #1086

Closed
wants to merge 3 commits into from
Closed

Fix 1.9 syntax error #1086

wants to merge 3 commits into from

Conversation

BanzaiMan
Copy link
Contributor

Description

0.1x is supposed to be 1.9-compat, so change this method signature to reflect it.

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation

Additional Notes

https://travis-ci.com/travis-ci/travis.rb/jobs/263075645#L271

0.1x is supposed to be 1.9-compat
@BanzaiMan
Copy link
Contributor Author

Not sure how to fix the CI.

BanzaiMan added a commit to travis-ci/travis.rb that referenced this pull request Dec 10, 2019
0.17.1 is incompatible with 1.9, so, until
lostisland/faraday#1086 or similar is released,
we need to stay on 0.17 for 1.9.
@olleolleolle
Copy link
Member

@BanzaiMan GitHub actions doesn't support 2.3 any longer. On master, that element of the ci matrix was dropped, to go green.

Such a PR is needed in this branch, too.

Support for github action of ruby 2.3 was removed. Now according to
https://github.com/actions/setup-ruby/blob/master/action.yml
theminimal version is 2.4.
@BanzaiMan
Copy link
Contributor Author

BanzaiMan commented Dec 10, 2019

Not sure why GitHub thinks 2.3 is still required and expected. ¯\_😕_/¯

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.

I'm not sure what we're trying to fix here, but

  1. I'm not sure Ruby 1.9 should still be supported in 0.1x
  2. The proposed change is incompatible with the existing code

@@ -85,7 +85,7 @@ def initialize(exc = 'timeout', response = nil)

# Raised by Faraday::Response::RaiseError in case of a nil status in response.
class NilStatusError < ServerError
def initialize(_exc, response: nil)
def initialize(_exc, response = nil)
Copy link
Member

Choose a reason for hiding this comment

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

This is NOT the same thing as def initialize(_exc, response: nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that's true. However, I will point out that this is the same call signature as that of TimeoutError on line 80. If this is not the right way, then TimeoutError should also be revisited.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be rewritten to accept a hash parameter? Like that:

def initialize(_exc, params)
  response = params[:response]

@iMacTia
Copy link
Member

iMacTia commented Dec 10, 2019

Well, according to the 0.1x README we do 😃! But no tests are executed against 1.9 anymore, so hard to catch this sort of things 🤔

@BanzaiMan
Copy link
Contributor Author

@iMacTia
Copy link
Member

iMacTia commented Dec 27, 2019

Unfortunately I was unable to get 1.9 working again on our GitHub Actions CI, so it's hard to say if we can or can't merge this PR.
Moreover we've consistently merged incompatible changes with 2.0, possibly more than the ones already highlighted in this PR.

@BanzaiMan are you still using Ruby 1.9 together with the latest Faraday version?
@olleolleolle @technoweenie latest releases (0.17.x, possibly sooner) have already broken the compatibility with 1.9, should we come back on track or just drop the official support?

@BanzaiMan
Copy link
Contributor Author

@iMacTia > are you still using Ruby 1.9 together with the latest Faraday version?
Yes. For many reasons, we need Faraday to provide a 1.9-compatible library in some form. I understand that 1.9 is certifiably EOL, but if it is no longer supported, the incompatible versions should be yanked so that we have some reasonable version that supports 1.9 can be found.

Looks to me that #1094 supersedes this, so I'll close this now.

@BanzaiMan BanzaiMan closed this Dec 27, 2019
@BanzaiMan BanzaiMan deleted the patch-1 branch December 27, 2019 22:24
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.

4 participants