panthomakos / timezone

Accurate current and historical timezones for Ruby with support for Geonames and Google latitude - longitude lookups.
http://rubygems.org/gems/timezone
MIT License
354 stars 49 forks source link

Use geonames by default with google lookup as fallback #73

Closed amnesia7 closed 7 years ago

amnesia7 commented 7 years ago

Hi,

I am updating my app to use a >1.0.0 version of the gem.

The app used geonames by default and then try using the google lookup if geonames returned an error.

It did this by only setting the google api key config variable when required and then setting it back as nil again afterwards so that the default was back as geonames again.

Can you advise how the new format of setting the gem's config, eg Timezone::Lookup.config(:google) can be used to use geonames for the lookup by default and google as a fallback?

I have this in my application initializer:

    Timezone::Lookup.config(:geonames) do |c|
      c.username = 'my_username'
      c.offset_etc_zones = true
      c.order_list_by = :utc_offset
    end

and then in my model I have the following so far as I convert it to the new syntax:

      begin
        # get timezone for lat,lng from Geonames API
        timezone = Timezone.lookup(self.latitude, self.longitude)
        self.time_zone = timezone.name
      rescue Timezone::Error::Base
        Timezone::Lookup.config(:google) do |c|
          c.api_key = ENV['GOOGLE_API_KEY']
        end
        # get timezone for lat,lng from Google API if Geonames API fails
        timezone = Timezone.lookup(self.latitude, self.longitude)
        self.time_zone = timezone.name
        # reset google_api_key after use
        Timezone::Lookup.config(:google) do |c|
          c.api_key = nil
        end
      end

Am I on the right track with converting this or will this set the lookup to always use google and if so how would I amend it to revert back to geonames each time?

Thanks

amnesia7 commented 7 years ago

Or would it be more like using config when required and not in the initializer in this case?

      begin
        # get timezone for lat,lng from Geonames API
        Timezone::Lookup.config(:geonames) do |c|
          c.username = 'my_username'
          c.offset_etc_zones = true
          c.order_list_by = :utc_offset
        end
        gn_timezone = Timezone.lookup(self.latitude, self.longitude)
        self.time_zone = gn_timezone.name
      rescue Timezone::Error::Base
        Timezone::Lookup.config(:google) do |c|
          c.api_key = ENV['GOOGLE_API_KEY']
        end
        # get timezone for lat,lng from Google API if Geonames API fails
        gg_timezone = Timezone.lookup(self.latitude, self.longitude)
        self.time_zone = gg_timezone.name
      end

My only other uses of the gem are for validating the timezone is in the full list of names which I don't think should need anything to be configured or am I better leaving the geonames config in the initializer as well anyway?

amnesia7 commented 7 years ago

Minor adjustment to the code above:

      begin
        # get timezone for lat,lng from Geonames API
        Timezone::Lookup.config(:geonames) do |c|
          c.username = 'my_username'
        end
        gn_timezone = Timezone.lookup(self.latitude, self.longitude)
        self.time_zone = gn_timezone.name
      rescue Timezone::Error::Base
        Timezone::Lookup.config(:google) do |c|
          c.api_key = ENV['GOOGLE_API_KEY']
        end
        # get timezone for lat,lng from Google API if Geonames API fails
        gg_timezone = Timezone.lookup(self.latitude, self.longitude)
        self.time_zone = gg_timezone.name
      end

And then I have this in my timezone.rb initializer just to be sure:

    Timezone::Lookup.config(:geonames) do |c|
      c.username = 'my_username'
    end

I think that should do it. @panthomakos if you can confirm then this issue can be closed.

Thanks

panthomakos commented 7 years ago

Setting the lookup config back and forth between geonames and google is not recommended because the code is not thread-safe when used that way (since the config is essentially global). If you are not concerned with thread safety, then that code looks fine to me.

I do have a plan to allow a fallback configuration in the gem itself. I just haven't gotten around to implementing this yet. I was thinking of something like:

Timezone::Lookup.config(:geonames) { ... }
Timezone::Lookup.config(:google) { ... }

Timezone.using(:geonames).lookup(lat, lng) # You would manually specify the config to use

or

Timezone::Lookup.add_config(:geonames) { ... }
Timezone::Lookup.add_config(:google) { ... }

Timezone.lookup(lat, lng) # would use geonames first then fallback to google
amnesia7 commented 7 years ago

Ah ok. I think your first option could be good. I assume .lookup would just use the first lookup service if .using wasn't specified. I think it leaves the ability for the order the services are used in the application to be changed rather than it being constrained within an initializer but that's just my thought, I can't really think of a good reason off the top of my head though.

panthomakos commented 7 years ago

I would like to retain full backwards compatibility with the existing API and I think that allowing the user to specify fallback behavior is probably best (rather than having timezone do the fallback). This would make it easy to incorporate exponential back-off or allow dynamic toggling of which service to use.

I also realized that the following is currently supported, and is thread-safe. I might simply document it for the next release:

geonames_lookup = ::Timezone::Lookup.config(:geonames) do |c|
  c.username = 'foobar'
end

google_lookup = ::Tiimezone::Lookup.config(:google) do |c|
  c.api...
end

geonames_lookup.lookup(lat, lng)
google_lookup.lookup(lat, lng)
amnesia7 commented 7 years ago

Should not mean that this should work because it doesn't seem to? Have I missed something somewhere?

      geonames_lookup = Timezone::Lookup.config(:geonames) do |c|
        c.username = 'foobar'
      end
      google_lookup = Timezone::Lookup.config(:google) do |c|
        c.api_key = ENV['GOOGLE_API_KEY']
      end

      begin
        tz = geonames_lookup.lookup(self.latitude, self.longitude)
        self.time_zone = tz.name
      rescue Timezone::Error::Base
        tz = google_lookup.lookup(self.latitude, self.longitude)
        self.time_zone = tz.name
      end
panthomakos commented 7 years ago

Can you clarify what is not working in your example?

amnesia7 commented 7 years ago

Sorry @panthomakos I never got around to getting back to you. I'll give it another go next time I get chance.

amnesia7 commented 7 years ago

@panthomakos would those constants (GEONAMES_LOOKUP and GOOGLE_LOOKUP) be declared at the top of my activerecord model class and then the begin-rescue-end be in the particular method that fetches the timezone from the API?

panthomakos commented 7 years ago

Yes - although you can re-create the lookups as well - it doesn't matter. The difference is that the last lookup you define will also be the default that is used by ::Timezone.lookup.