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

Add Graphlient::Errors::ConnectionFailedError #68

Merged
merged 3 commits into from
Nov 13, 2019
Merged

Add Graphlient::Errors::ConnectionFailedError #68

merged 3 commits into from
Nov 13, 2019

Conversation

neroleung
Copy link
Contributor

This is essentially #59 with test.

Whenever there is an Errno::ECONNREFUSED error, Faraday wraps it in a Faraday::ConnectionFailed error. Since it inherits Faraday::ClientError without a response (see errors/faraday_server_error.rb), the code would return a NoMethodError: undefined method []' for nil:NilClass error.

So basically the fix is to rescue Faraday::ConnectionFailed before rescuing Faraday::ClientError so that we can raise a more meaningful error.

Result:

Graphlient::Errors::ConnectionFailedError: Failed to open TCP connection to localhost:3000 (Connection refused - connect(2) for "localhost" port 3000)

This is essentially #59 with test.

Whenever there is an `Errno::ECONNREFUSED error`, Faraday wraps it in a
`Faraday::ConnectionFailed` error. Since it inherits `Faraday::ClientError`
without a response (see errors/faraday_server_error.rb), the code would return
a `NoMethodError: undefined method []' for nil:NilClass` error.

So basically the fix is to rescue `Faraday::ConnectionFailed` before rescuing
`Faraday::ClientError` so that we can raise a more meaningful error.

Result:
```
Graphlient::Errors::ConnectionFailedError: Failed to open TCP connection to
localhost:3000 (Connection refused - connect(2) for "localhost" port 3000)
```
Copy link
Collaborator

@dblock dblock 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 good with this pending the minor changelog entry comment.

CHANGELOG.md Outdated Show resolved Hide resolved
stub_request(:post, url).to_raise(error)
end

specify do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fancy :)

@dblock
Copy link
Collaborator

dblock commented Nov 13, 2019

Oh one thing. You should add an UPGRADING document in this PR like this one because this does change the interface. If someone was handling the Faraday error they now get a ServerError.

@neroleung
Copy link
Contributor Author

@dblock, I've updated CHANGELOG.md and README.md. Also added UPGRADING.md. Question though, do you want a new section in README.md like https://github.com/dblock/slack-ruby-client#stable-release?

@dblock dblock merged commit 4935dfc into ashkan18:master Nov 13, 2019
@dblock
Copy link
Collaborator

dblock commented Nov 13, 2019

This is perfect, thank you. Feel free to PR something linking to UPGRADING.

@neroleung
Copy link
Contributor Author

Thanks. This should close #51, #52 and #59 once a new version is released.

@dblock
Copy link
Collaborator

dblock commented Nov 14, 2019

I'll cut a release unless @yuki24 or @ashkan18 can beat me to it (please).

@ashkan18
Copy link
Owner

thanks @neroleung ! this is awesome! going to cut the release, hopefully before @dblock 😄

@ashkan18
Copy link
Owner

alright 0.3.7 is now published.

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