-
Notifications
You must be signed in to change notification settings - Fork 125
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 faraday connectionfailed issue #178
Fix faraday connectionfailed issue #178
Conversation
While all tests passed on Travis CI, some tests failed on Appvoyer. |
The failures were due to flaky tests. They have now passed when re-run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gem locks faraday to 1.x, which may cause breakages in gems used in tandem in Jekyll projects. I’d be interested in finding a way to detect if Faraday::Error exists and use the proper error class. I have used arrays of class names in the past which is useful here since they can be dynamically defined.
We could also lock Faraday to <1.0 for now to help folks out now, but that might cause confusion because it may hold back other gems, too.
# NOTE: Faraday's error classes has been promoted to under Faraday module from v1.0.0. | ||
# This patch aims to prevent on locking specific version of Faraday. | ||
FARADAY_FAILED_CONNECTION = | ||
begin | ||
Faraday::Error::ConnectionFailed | ||
rescue NameError | ||
Faraday::ConnectionFailed | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parkr this tries to detect whether Faraday::Error::ConnectionFailed
works or not. This would prevents from locking specific version of Faraday. All tests passes for the both versions v0.17 and v1.0 of Faraday: Build #517 - jekyll/github-metadata - Travis CI.
Testing with Ruby 2.6 (or latest), Jekyll 4.x and Faraday 1.x is already the default job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Thank you for your contribution @satoryu. @jekyllbot: merge +dev |
following guidelines from jekyll/github-metadata#178
This pull-request is to close #177