dry-rb / dry-logic

Predicate logic with rule composition
https://dry-rb.org/gems/dry-logic/
MIT License
179 stars 66 forks source link

Add respond_to? predicate #55

Closed waiting-for-dev closed 5 years ago

waiting-for-dev commented 5 years ago

This is part of a feature for dry-types about being able to define a type by a contract of implemented methods in the value.

Calling the predicate method respond_to? has the undesired side effect of obfuscating Object#respond_to? in Dry::Logic::Predicates. An alternative would be to name it responds_to?, but it would be less intuitive for users who would find more natural to have the predicate named like the method. Another solution would be to decouple predicate names from Dry::Logic::Predicates method names. What do you think?

Resources

solnic commented 5 years ago

Calling the predicate method respond_to? has the undesired side effect of obfuscating Object#respond_to? in Dry::Logic::Predicates.

This is not a problem. This module is a placeholder for predicates, the actual API are predicates themselves, not the module that contains them.

dgutov commented 4 years ago

@solnic It is a problem for code that performs introspection on the running program which has dry-logic loaded.

solnic commented 4 years ago

@dgutov wdym? it does not affect anything outside of dry-logic.

flash-gordon commented 4 years ago

dry-logic are not meant to be introspected anyway, not with respond_to?. Any library relying on reflection should have escape hatches for such situations. For instance, dry-logic rules can be dumped to AST, this makes a lot more sense that probing them with respond_to?.

dgutov commented 4 years ago

@solnic It breaks application code like:

ObjectSpace.each_object(Module).select { |m| m.respond_to?(:dry) }

I have a plucky little code assistance tool which (very roughly speaking) uses this approach.

solnic commented 4 years ago

@dgutov oh boy 😭 well, please report an issue then. This is something we can change in 2.0.0.

flash-gordon commented 4 years ago

For the record, changing it may affect performance since method dispatching is optimized in ruby as well as in dry-logic

solnic commented 4 years ago

@flash-gordon I reckon we can live with that, I suspect it's not a commonly used predicate

flash-gordon commented 4 years ago

@solnic ah, no I thought of something more fundamental like changing the way we group predicates, e.g. having them in a hash rather than a module

solnic commented 4 years ago

@flash-gordon ah no, we should just rename it to something like...I dunno, implements?: :foo O_o coming up with a name won't be easy ;)

flash-gordon commented 4 years ago

@dgutov a work around would be

respond_to = Class.instance_method(:respond_to?)
ObjectSpace.each_object(Module).select { |m| respond_to.bind_call(m, :dry) }

bind_call is 2.7+ IIRC but you can .bind(m).(:dry) in earlier versions

dgutov commented 4 years ago

@flash-gordon This is a decent idea, but it enters murky waters. respond_to? is a method that is actually supposed to be redefined by classes. Just in a compatible fashion.

And the particular line where I'm getting an error because of this addition to dry-logic was added to deal with a quirk in JRuby where some instances of Module don't implement the method included_modules. A respond_to? check has been stable. I'm not sure how they would behave with a respond_to? method transplanted from Module, or whether that would change in some future version.

dgutov commented 4 years ago

@solnic Reported. ;)

solnic commented 4 years ago

@dgutov thanks! I added it to 2.0.0 milestone