Use existing google_request_denied.txt mock

[AlexWayfer]

Don't suppress any errors, please.

[panthomakos]

Can you give a little more detail on this change? What exactly is being accomplished here? The errors are not being suppressed, just wrapped in a Timezone::Error::GeoNames exception.

[AlexWayfer]

OK, some example:

  1) Error:
TestGoogle#test_google_using_lat_long_coordinates:
Timezone::Error::Google: 751: unexpected token at '{
   "dstOffset" : 3600,
   "rawOffset" : 34200,
   "status" : "OK",
   "timeZoneId" : "Australia/Adelaide"
   "timeZoneName" : "Australian Central Daylight Time"
}

And the real case in this PR (from test_google_request_denied_read_lat_long_coordinates if comment assert_raises and just do mine.lookup):

  1) Error:
TestGoogle#test_google_request_denied_read_lat_long_coordinates:
Timezone::Error::Google: no implicit conversion of nil into String
    /home/alex/Projects/ruby/timezone/lib/timezone/lookup/google.rb:48:in `rescue in lookup'
    /home/alex/Projects/ruby/timezone/lib/timezone/lookup/google.rb:31:in `lookup'
    /home/alex/Projects/ruby/timezone/test/timezone/lookup/test_google.rb:45:in `test_google_request_denied_read_lat_long_coordinates'

This is not exception what we're waiting in this test-case:

assert_raises Timezone::Error::Google, 'The provided API key is invalid.' do

[panthomakos]

The rescue clause changes you have made do not resolve any underlying issues. When the rescue clauses are retained, the tests still pass. I think the change in the test case is good, but removing the rescue clause does not resolve an issue and is not backwards compatible.

[AlexWayfer]

The rescue clause changes you have made do not resolve any underlying issues. When the rescue clauses are retained, the tests still pass.

It resolves:

1. Unclear exceptions (example above, but it also can be exception from the Net::HTTP)
2. Tests with incorrect Timezone::Error::Google (by JSON parsing, not from JSON content) are passing with rescue, and it's wrong.

I have not met anywhere else such wrapping of any error, I think it's bad practice.

[AlexWayfer]

Now in our production env:

Timezone::Error::Google: execution expired

Execution of what expired? Not so clear.

[AlexWayfer]

begin
  @google_lookup.lookup(*args)
rescue Net::OpenTimeout
  @geonames_lookup.lookup(*args)
end

Timezone::Error::Google even isn't a descendant of Net::OpenTimeout, and it raising 😫

If I'll rescue Timezone::Error::Google — it'll rescue any Exception, and that's wrong!

https://robots.thoughtbot.com/rescue-standarderror-not-exception

Please!

[AlexWayfer]

OK, in the last version of master any StandardError, not Exception. But I need to rescue only Net::OpenTimeout, and I can't do it now.

Branch is updated.

[panthomakos]

The recommended way to manage network exceptions/cases beyond what is provided by default in this gem is to write your own http request handler: https://github.com/panthomakos/timezone#using-your-own-http-request-handler.

I would be open to expanding the set of functionalities/APIs that the request handler manages. For example we could allow handlers to raise exceptions that don't get wrapped, or we could modify the internal client handling code so that some handlers can continue to raise exceptions.

Another option would be to expand the timezone errors to also have a reference to orig_exception or original_exception so that the original exception can be retrieved in a backwards compatible manner.

A final, yet not mutually exclusive option, would be to create another subclass of errors that are network specific but that can be rescued in a backwards compatible manner as well.

I want to think about the API in a little more detail before deciding on anything. In an ideal world, if there is going to be a backwards compatible change, I would probably want to re-design the http handlers so that they are responsible for providing a response body (or body and success/failure boolean) to the lookup object. That way the handlers could have their own set of exceptions/responses that they can raise/return independently of the lookup code.

[AlexWayfer]

The recommended way to manage network exceptions/cases beyond what is provided by default in this gem is to write your own http request handler: https://github.com/panthomakos/timezone#using-your-own-http-request-handler.

It's overhead for a simple exceptions catching.

I would be open to expanding the set of functionalities/APIs that the request handler manages. For example we could allow handlers to raise exceptions that don't get wrapped, or we could modify the internal client handling code so that some handlers can continue to raise exceptions.

I don't see a need for internal errors.

Another option would be to expand the timezone errors to also have a reference to orig_exception or original_exception so that the original exception can be retrieved in a backwards compatible manner.

There is Exception#cause. But it's harder to catch/rescue: you should rescue any error, then check #cause in rescue, then re-raise or do something.