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 args forwarding specs #756

Merged
merged 1 commit into from
Feb 15, 2020
Merged

Conversation

palkan
Copy link
Contributor

@palkan palkan commented Feb 12, 2020

Related to #745.

This is a basic args forwarding spec (extracted from ruby-next).

I tried to add a spec for brackets-less case like this:

it "treats arguments as Range when brackets are ommitted" do
  a = Class.new(F)
  a.class_eval(<<-RUBY)
      def delegate(...)
        target ...
      end
   RUBY

   # this passes
   a.new.delegate(1, b: 2).should == [[], {}]
end

But I'm not sure why the result is empty (testing in 2.7.0)? Shouldn't there be a beginless-endless range?

@palkan palkan force-pushed the feat/args-forwarding branch from e178432 to 32fec93 Compare February 13, 2020 12:44
@@ -0,0 +1,9 @@
class F
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you nest this under DelegationSpecs and name it more explicitly like Target?
That will also fix the CI.

@@ -0,0 +1,28 @@
require_relative '../spec_helper'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filename has a typo (missing d).
I think this should be called delegation_spec.rb and later on we could extend specs in this file with how delegation is achieved in various versions.

@eregon
Copy link
Member

eregon commented Feb 13, 2020

Re the parentheses-less version, I see this (on 2.7.0):

class F
  def target(*args, **kwargs)
    [args, kwargs]
  end
end

a = Class.new(F)
a.class_eval(<<-RUBY)
def delegate(...)
  target ...
end
RUBY

p a.new.delegate(1, b: 2)
# => [[], {}]...

So it's the same as target()....
That would be good to spec, yes.

@palkan palkan force-pushed the feat/args-forwarding branch from 32fec93 to 9f95636 Compare February 13, 2020 23:52
@palkan palkan force-pushed the feat/args-forwarding branch from 9f95636 to 1ce89bb Compare February 13, 2020 23:53
@palkan
Copy link
Contributor Author

palkan commented Feb 13, 2020

Thanks for the review! Pushed the updated version.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@eregon eregon merged commit 10fe05c into ruby:master Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants