Adds specific errors for connection timeout and failure

[Maimer]

This should be backwards compatible with the existing code as the new errors inherit from ApiError and maintain the same string signature of the previous errors.

Addresses the following issue: #428

@phoenixy1

[Maimer]

@phoenixy1 thoughts on this?

[phoenixy1]

Thanks or the PR! I'm not really a rubyist -- I'll ask around internally for review but I would also appreciate opinions on this change from others users of this library. Cc: @kyledcline @ctlevi @enriquez @pradyumna2905 @bdewater

[bdewater]

+1 to this, it’s easier and idiomatic to rescue a specific subclass rather than matching on a string message.

[pdx91]

Yep, this makes sense to me. Backward compatible too! 👍

[phoenixy1]

ok, sounds like the people approve! I'll put this into the queue to work on -- hopefully we can get it in for the next clib release.

[phoenixy1]

@Maimer I tried importing this upstream but the test suite will not run with this change and generates the following error:

/usr/src/app/lib/plaid/api_client.rb:26:in `<class:ApiClient>': uninitialized constant Plaid::ApiClient::ApiError (NameError)

Can you fix whatever it's missing (or give instructions on the fix it needs, if it's not part of the published clib)? Thanks!

[rubendinho]

Re #483 (comment), I believe it's a namespace issue

Suggested change

	class ApiTimeoutError < ApiError; end
	class ApiConnectionFailedError < ApiError; end
	class ApiTimeoutError < Plaid::ApiError; end
	class ApiConnectionFailedError < Plaid::ApiError; end

Or may need to be ::Plaid

[Maimer]

I believe it's a loading issue, which is only present in the tests. The namespace should work without needing to be explicit.

[phoenixy1]

I did try changing the namespace as suggested here but now get the error:

/usr/src/app/lib/plaid/api_client.rb:26:in `<class:ApiClient>': uninitialized constant Plaid::ApiError (NameError)

If you can figure out the solution that would be great. If not, things have been a little hectic this week (performance review season, family emergency) but I can also ask some people who actually know Ruby to take a look sometime next week.

[Maimer]

I did try changing the namespace as suggested here but now get the error:

/usr/src/app/lib/plaid/api_client.rb:26:in `<class:ApiClient>': uninitialized constant Plaid::ApiError (NameError)

If you can figure out the solution that would be great. If not, things have been a little hectic this week (performance review season, family emergency) but I can also ask some people who actually know Ruby to take a look sometime next week.

@phoenixy1 Apologies as I had meant to take a look at this sooner. I have pushed what I believe to be a working solution to the NameError. I believe that the test suite could be cleaned up a bit more and be made more conventional. However, I have not setup the test suite to run entirely on my machine. I did get the specific test I wrote to pass when run on its own. It would be nice if this test suite were either easier to run locally, or had a public facing CI run that I could use to verify my work. If that were the case I could spend some time standardizing the way this all works and make it easier to make changes going forward. How are the tests currently run and changes verified internally? All I see is the build step waiting in CircleCI. Would it be possibe to make that build public?

[phoenixy1]

Thanks! I tried this and the tests pass with these changes; I've submitted a fix for upstream.

I don't have a ton of insight into CI for this repo -- honestly, I don't have access to CircleCI either and typically run the tests locally using our internal makefile, which builds the libraries using the Docker images provided by OpenApiTools and then runs the tests. Anyway, I can pass the request along to the team that does run it!

[Maimer]

Thanks! I tried this and the tests pass with these changes; I've submitted a fix for upstream.

I don't have a ton of insight into CI for this repo -- honestly, I don't have access to CircleCI either and typically run the tests locally using our internal makefile, which builds the libraries using the Docker images provided by OpenApiTools and then runs the tests. Anyway, I can pass the request along to the team that does run it!

Awesome, glad it worked out and will be in the next release!

Thanks for passing along the CI request. At my company (AppFolio) we also use CircleCI and all of our open source repos have publicly available CI.

[Maimer]

@Maimer I tried importing this upstream but the test suite will not run with this change and generates the following error:

/usr/src/app/lib/plaid/api_client.rb:26:in `<class:ApiClient>': uninitialized constant Plaid::ApiClient::ApiError (NameError)

Can you fix whatever it's missing (or give instructions on the fix it needs, if it's not part of the published clib)? Thanks!

I'll take a look at this and hopefully have a fix for you soon.

[phoenixy1]

This has been merged into upstream and should be released with the next scheduled ruby client library release!