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

adds spec for an empty keyword splat to a method that does not accept… #799

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

moofkit
Copy link
Contributor

@moofkit moofkit commented Oct 14, 2020

… keywords

Closing #745

Passing an empty keyword splat to a method that does not accept keywords
no longer passes an empty hash, unless the empty hash is necessary for
a required parameter, in which case a warning will be emitted. Remove
the double splat to continue passing a positional hash. Feature #14183

h = {}; def foo(*a) a end; foo(**h) # []
h = {}; def foo(a) a end; foo(**h)  # {} and warning
h = {}; def foo(*a) a end; foo(h)   # [{}]
h = {}; def foo(a) a end; foo(h)    # {}

@andrykonchin
Copy link
Member

Looks good but there is no test for this case:

h = {}; def foo(*a) a end; foo(**h) # []

suppress_warning do
m(**h).should == {}
end
end.should_not raise_error
Copy link
Member

@andrykonchin andrykonchin Oct 15, 2020

Choose a reason for hiding this comment

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

Checking for not raising exception is excessive to it's a common practice in RubySpec not to use it at all. If exception is raised a test will fail anyway.

@andrykonchin
Copy link
Member

Also we can test the warning itself in this case

h = {}; def foo(a) a end; foo(**h)  # {} and warning

@moofkit
Copy link
Contributor Author

moofkit commented Oct 15, 2020

@andrykonchin Thanks for review!

Looks good but there is no test for this case:

h = {}; def foo(*a) a end; foo(**h) # []

Yeah, I was started the spec because of comment, that behavior was changed. But when I checked with ruby 2.7 the result was same to 2.6. I think may be there was some mistake... Now I checks this one more time and it's really was changed 😅

Thanks for point this!

@andrykonchin
Copy link
Member

Great 👍

@andrykonchin andrykonchin merged commit 4318316 into ruby:master Oct 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