Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add respond_to? predicate #55

Merged
merged 1 commit into from
May 4, 2019

Conversation

waiting-for-dev
Copy link
Member

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 solnic merged commit fab9031 into dry-rb:master May 4, 2019
@solnic
Copy link
Member

solnic commented May 4, 2019

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.

waiting-for-dev added a commit to waiting-for-dev/dry-rb.org that referenced this pull request May 4, 2019
@dgutov
Copy link

dgutov commented Jul 28, 2020

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

@solnic
Copy link
Member

solnic commented Jul 29, 2020

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

@flash-gordon
Copy link
Member

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
Copy link

dgutov commented Jul 29, 2020

@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
Copy link
Member

solnic commented Jul 30, 2020

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

@flash-gordon
Copy link
Member

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

@solnic
Copy link
Member

solnic commented Jul 30, 2020

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

@flash-gordon
Copy link
Member

@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
Copy link
Member

solnic commented Jul 30, 2020

@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
Copy link
Member

@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
Copy link

dgutov commented Jul 30, 2020

@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
Copy link

dgutov commented Jul 30, 2020

@solnic Reported. ;)

@solnic
Copy link
Member

solnic commented Jul 31, 2020

@dgutov thanks! I added it to 2.0.0 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants