search

sourcelevel / faraday-http-cache

A Faraday middleware that respects HTTP cache
Other
343 stars 86 forks source link

Add cache key configuration option #97

Closed ryuichi7 closed 5 years ago

ryuichi7 commented 6 years ago

This is a proposal to add a new configuration option called :cache_key_callback which allows customization of the cache_key by passing a Proc that takes the request object as an argument.

I've added an example to the README for clarity.

It seems that in the past, the entire request object was passed to the private #cache_key_for method but was amended to pass only the request.url due to issues with cache invalidation which was brought up here: https://github.com/plataformatec/faraday-http-cache/issues/42

Because the request.url is the only attribute ever used as a cache_key I was wondering if the code here is necessary? I couldn't find any place where the Location or Content-Location header values are used as a cache_key. That's why I removed it, but maybe I'm missing something?

Thanks so much!

sourcelevel-bot[bot] commented 6 years ago

Hello, @ryuichi7! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

sourcelevel-bot[bot] commented 6 years ago

Ebert has finished reviewing this Pull Request and has found:

You can see more details about this review at https://ebertapp.io/github/plataformatec/faraday-http-cache/pulls/97.

lucasmazza commented 5 years ago

Thanks for your Pull Request but I don't think its a good idea to have a configuration for this, because it will work as a escape hatch to "cache requests as you see fit" and not to be closer to the specs we are trying to follow here.