Conversation
| class StoreProxy < SimpleDelegator | ||
| class << self | ||
| def proxies | ||
| @@proxies ||= [] |
There was a problem hiding this comment.
Would instance variable be enough here?
There was a problem hiding this comment.
No, should be class variable to work with chains of inheritance, like class RedisProxy < StoreProxy, class RedisStoreProxy < RedisProxy
There was a problem hiding this comment.
Gotcha.
Was thinking under the assumption all proxies inherited directly from StoreProxy, forgot that chained redis case.
spec/rack_attack_dalli_proxy_spec.rb
Outdated
| describe Rack::Attack::StoreProxies::DalliProxy do | ||
| it 'should stub Dalli::Client#with on older clients' do | ||
| proxy = Rack::Attack::StoreProxy::DalliProxy.new(Class.new) | ||
| proxy = Rack::Attack::StoreProxies::DalliProxy.new(Class.new) |
There was a problem hiding this comment.
I know this is not "officially" part of the public API, but given it might be easy to avoid in this case, can we please try not breaking any user code that references the current proxies, by keeping constant refs the same as before?
Maybe the StoreProxy class you added can become BaseProxy or just Base?
There was a problem hiding this comment.
Yes, sure this can be make. But if we will consider original part of this extracted pr, 1) this namespace will still be changed to something like StoreAdapters and 2) personally, new names (StoreProxies namespace and StoreProxy base class) looks cleaner to me than Base or BaseProxy.
So, taking into account first mentioned reason, should I still preserve original namespace name?
fatkodima
force-pushed
the
dynamic-proxies
branch
from
addd4ed to
edaa6c6
Compare
|
Ignore my last comment. |
| # frozen_string_literal: true | ||
|
|
||
| require 'delegate' | ||
| require 'rack/attack/store_proxy/redis_proxy' |
|
👏 |
|
Released in |
2 participants
#441 (comment)