Introduce Arproxy::Proxy: incompatible but keep functionalities over v0.x#34
Introduce Arproxy::Proxy: incompatible but keep functionalities over v0.x#34mirakui merged 10 commits intocookpad:mainfrom
Conversation
lib/arproxy/config.rb
Outdated
|
|
||
| def use(proxy_class, *options) | ||
| if proxy_class.is_a?(Class) && proxy_class.ancestors.include?(Arproxy::Base) | ||
| # TODO: Add a migration guide URL |
There was a problem hiding this comment.
I will add an url like following into the error message after merging
There was a problem hiding this comment.
nit: How about creating a new file, say UPGRADING.md, and moving this guide there? Then we can add an URL before merging, without including a commit hash in the URL
nekketsuuu
left a comment
nekketsuuu
left a comment
There was a problem hiding this comment.
Looks almost good to me -- just leaving some comments for confirmation
lib/arproxy/config.rb
Outdated
|
|
||
| def use(proxy_class, *options) | ||
| if proxy_class.is_a?(Class) && proxy_class.ancestors.include?(Arproxy::Base) | ||
| # TODO: Add a migration guide URL |
There was a problem hiding this comment.
nit: How about creating a new file, say UPGRADING.md, and moving this guide there? Then we can add an URL before merging, without including a commit hash in the URL
| end | ||
|
|
||
| def with_binds? | ||
| !!@with_binds |
There was a problem hiding this comment.
imo: How about defining #with_binds? as !!@binds and set binds: nil in the initializer for non-bind executions? Currently QueryContext#binds is meaningless when !with_binds? and it is less intuitive for users I think
There was a problem hiding this comment.
I also considered the same thing, but I wonder if some adapter might use binds: nil for something in the future.
There was a problem hiding this comment.
@nekketsuuu is correct. @with_binds is used to identify whether the target method has binds as an argument. This value needs to be managed separately from whether it is nil or not.
e.g. An execute method without binds:
with binds:
3 participants
The upgrade from v0 to v1 via #32 was not compatible with the behavior of the proxy, and did not cover the use cases of Arproxy as before.
In this PR, I have given up on backward compatibility in the proxy interface and changed the policy so that existing use cases can be covered.
In order to make users aware of this change, I have changed the base class of the proxy from
Arproxy::BasetoArproxy::Proxyand made it so that an exception is thrown when using the former.References