Cache values for historical queries

[zapnap]

This was something we really needed for a project. Thoughts and suggestions welcome.

[houndci-bot]

Pass &:set as an argument to dont_allow instead of a block.

[vlado]

Hi @zapnap, I was actually thinking to implement something like this in a near future but looks like you overtake me :)

I'm in a big squeeze currently so just give me few days to review the pull request.
Thanks

[zapnap]

Great thanks! lmk.

[vlado]

Hi @zapnap, I finally got time for a review. Sorry for waiting so long.

I think configuration could be simplified if we replace config.cache.type and config.cache.client with only one config option, for example config.cache_store. Config could then look like this:

OpenExchangeRates.configure do |config|
  config.app_id = "YourAppID"
  config.cache_store = :memory
end

If symbol or string is defined we use it to get class name. :memory would map to OpenExchangeRates::Cache::MemoryAdapter, and if you want something custom you just set it to the instance of the class. Redis.new(url: 'redis://localhost:6379') or MyCustomExchangeRatesCacheAdapter.new for example.

I would move adapters from open_exchange_rates/cache.rb file to own files. Base adapter to open_exchange_rates/cache/base_adapter.rb, memory adapter to open_exchange_rates/cache/memory_adapter.rb, etc. I think this will make it easier to add new adapters and for people to write their own adapters.

In Base I would define get and set to raise NotImplemetedError.

Maybe it would be good idea to write Rails adapter. It would map to Rails.cache http://guides.rubyonrails.org/caching_with_rails.html, and if you for example set Rails cache to use Redis then all you have to do is config.cache_store = :rails and you use Redis also for open exchange rates (through Rails cache). You only need to config cache store once.

Do we need expire at option and/or method to clean up the cache? In most cases you would want to keep rates in cache but what if cache gets full? Should we leave that to the users or should we add something like config.cache_expires_in = 10000 and/or OpenExchangeRates::Cache.clear?

I'm not sure how Thread.current memory adapter will behave if this is used with Puma or Thin as described here https://github.com/steveklabnik/request_store. Maybe we should add note to README about this when memory adapter is used.

Please fix Hound (Rubocop) https://houndci.com/configuration#ruby issues also. If some rule is not logical to you we can discuss it.

Let me know what you think.

[zapnap]

These are good questions, and definitely should be addressed (except for some of the hound issues ;-). A couple quick comments:

• I should have named the memory store a noop store or null store; it certainly isn't intended for production use but rather for testability
• Fair point on moving cache adapters to independent files; that would be the logical way to expand on them. Given that there really was only one functional (non-test) one, I kept them compact at the moment. Nbd.
• Other options including expires are a good idea; was just trying to keep initial proposal limited in scope (and is another reason I wouldn't simplify the config option namespace, personally).
• Rails cache adapter is a good idea but is probably a separate feature imo

Unfortunately, I don't a whole lot of time at the moment (actually, no time at all). But will keep this in mind.