Skip to content

[WIP] Add dynamic callable and member lookup #27

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

toastersocks
Copy link

This adds dynamic callable and member lookup. I uncommented the commented out tests in TestDynamic.swift and they pass. Is this something you'd be open to merging?
I can add the dynamic call syntax to a section in the README.md if you'd like.

@codecov-io
Copy link

Codecov Report

Merging #27 into master will decrease coverage by 1.01%.
The diff coverage is 36.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   97.85%   96.84%   -1.02%     
==========================================
  Files          25       25              
  Lines        1775     1805      +30     
==========================================
+ Hits         1737     1748      +11     
- Misses         38       57      +19     
Impacted Files Coverage Δ
Sources/RubyGateway/RbGateway.swift 95.87% <0.00%> (-2.02%) ⬇️
Sources/RubyGateway/RbObject.swift 80.76% <39.28%> (-11.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc75929...e55c083. Read the comment docs.

@toastersocks
Copy link
Author

Hmm, I'll add a test for the @dynamicCallable code that's not currently covered.

@johnfairh
Copy link
Owner

Thanks for the PR. I would like to include some kind of support for this but I've never quite worked out a satisfactory model.

The part of this PR's approach that goes wrong is to do with methods with default arguments. Minimally consider:

class C
  def f(a = 2)
    a
  end
end

So C.new.f(1) is 1.

But because the approach 'eagerly' evaluates f before it sees the (1) it evaluates C.new.f(2)(1) instead.

I think my least bad solution to this ambiguity was to drop the eager dynamic member lookup part, forcing users to always write explicit method calls (). So C.new.f would yield some new 'partial method call' type that would support dynamic callable. I remember thinking it should be a separate type -- rather than an RbObject for the method for example -- to catch accidental misuse at compile-time. This design is unsatisfactory because it prevents the idiomatic Ruby omission of unnecessary (), though it is probably an improvement over the current call("methodname") pattern.

Perhaps your fresh eyes can see a better way through this!

@toastersocks
Copy link
Author

Oh, right. It didn't even occur to me that methods with default arguments would behave differently. Hmm, yeah I see what you mean about it not being ideal to have to put parentheses on every Ruby method that idiomatic Ruby treats more like properties. It does seem more "Swifty" to me though. As does getting a method reference back without the parens that you could potentially call later with parens.
My first approach months ago was to use a separate type for callables, but I abandoned it because I didn't like the extra complexity, and I realized I could get a method reference using the Ruby runtime and still return a 'RbObject'. My Ruby knowledge is pretty basic. Maybe we can use the Ruby runtime to check for default arguments? This would keep things nicer at the call site, allowing you to call methods that take no arguments without parens. I'll look into this more.

@toastersocks
Copy link
Author

toastersocks commented Oct 12, 2020

Actually, thinking about this more, It would add a lot of idiosyncrasy in that you'd have to only use parens to call a method if all its arguments have default values. Also some methods without parens would give you a method reference, and some would give you the result of the method, without a very good way to know beforehand without carefully checking the method signature and keeping these non-Ruby/non-Swift rules in mind.
As a user, I think I would favor the consistency and more Swift-like behavior of a method name without parens gives you a method reference, and to get the result, you need the parens. As I said, my Ruby is pretty basic, so I don't know if this would make things too cumbersome with a lot of common Ruby APIs.

@toastersocks
Copy link
Author

And, I just realized this doesn't handle named parameters at all... I'm going to flesh this out more.
Thanks for looking at this.

@toastersocks toastersocks marked this pull request as draft October 12, 2020 02:54
@toastersocks toastersocks changed the title Add dynamic callable and member lookup [WIP] Add dynamic callable and member lookup Oct 12, 2020
@johnfairh
Copy link
Owner

johnfairh commented Oct 12, 2020

favor [...] a method name without parens gives you a method reference

I agree with this. I think it's important that this type not be RbObject (it could wrap an RbObject & support RbObjectConvertible) so that the Swift type system prevents users from forgetting to add the parens, which would be a natural mistake coming from Ruby / porting Ruby code.

e: thinking about it, RbObjectConvertible compliance is probably not good for the same reason, too easy to accidentally use as an argument.

named parameters

Yes it should just be boilerplate to add the call variants once the object model is sound.

Base automatically changed from master to main February 5, 2021 11:34
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.

3 participants