I did some digging, and have been able to fix this behaviour in Do::All. In that context, we can use Module.private_instance_methods & Module.protected_instance_methods to determine what visibility each method has, and explicitly provide that visibility to the wrapped methods.
Like this: (from all.rb)
# @api private
module InstanceMixin
# @api private
def extended(object)
super
wrapper = ::Module.new
object.singleton_class.prepend(wrapper)
object.define_singleton_method(:singleton_method_added) do |method|
super(method)
next if method.equal?(:singleton_method_added)
visibility = if private_method_defined?(method, true)
:private
elsif protected_method_defined?(method, true)
:protected
else
:public
end
Do.wrap_method(wrapper, method, visibility)
end
object.singleton_class.public_instance_methods(false).each do |m|
Do.wrap_method(wrapper, m, :public)
end
object.singleton_class.protected_instance_methods(false).each do |m|
Do.wrap_method(wrapper, m, :protected)
end
object.singleton_class.private_instance_methods(false).each do |m|
Do.wrap_method(wrapper, m, :private)
end
end
end
Do.for has an additional challenge: for it to work properly, you need to specify which methods to wrap ahead of time - and that means we donāt know what visibility they will have once theyāre actually provided.
The only way I can think of to resolve that dilemma is to extend the API for Do.for to accept both a list of methods (that will be treated as public, per current behaviour), and keywords for explicit visibility:
# do.rb - L86
def for(*public_methods, **methods_with_visibility)
mod = ::Module.new do
public_methods.each do |method_name|
Do.wrap_method(self, method_name, nil)
end
methods_with_visibility.each do |visibility, methods|
methods.each do |method_name|
Do.wrap_method(self, method_name, visibility)
end
end
end
::Module.new do
singleton_class.send(:define_method, :included) do |base|
base.prepend(mod)
end
end
end
Usage:
class Example
include Dry::Monads[:result]
include Dry::Monads::Do.for(:call, :internal_method) # [1] current behaviour
include Dry::Monads::Do.for(:call, private: [:internal_method]) # [2] proposed API
def call(value)
result = yield internal_method(value)
end
private
def internal_method(v)
i = yield interstitial_result.to_monad
Success(v + i)
end
def interstitial_result
Success(100)
end
end
e = Example.new
e.call(1) # => Success(101)
# when [1]:
e.internal_method # => Success(101)
# when [2]:
e.internal_method # => NoMethodError (private method `internal_method' called for #<Example:0xdeadbeef>
I think the intention behind this API was that youād only decorate one public method. As you noticed, fixing it would be a challenge and I honestly donāt think itās worth the effort. OTOH it would probably be relatively simple to rely on method_added hook and decorate methods there (which would allow you to inspect visibility and act accordingly).
Iāll put what Iāve done up as a PR and you can decide whether itās worth a merge. If we decide not to try and fix Do.for, I think itās worth putting an explanation in the docs to highlight this surprising behaviour. WDYT?
All of these visibility problems should be fixed, Iāll take a closer look soon. Personally, I donāt pay attention to visibility since most of my classes have only one āpublicā, namely call.
While I usually only have one public call method, I use do notation a lot to clean up private methods too, and I donāt want that to make them public.