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

[Ruby 2.7] Add specs for pattern matching #751

Merged
merged 6 commits into from
Jan 17, 2020

Conversation

@eregon
Copy link
Member

eregon commented Jan 13, 2020

Great, thank you for starting those!
We probably need to wrap them in eval <<RUBY to avoid syntax error on old versions.
I already ticked it in #745 to mark it's in progress.

@andrykonchin andrykonchin force-pushed the andrykonchin-ruby-2-7--pattern-matching branch 2 times, most recently from 0a4a73c to 2998958 Compare January 15, 2020 21:37
@andrykonchin
Copy link
Member Author

@eregon

We probably need to wrap them in eval <<RUBY to avoid syntax error on old versions.

Yeah, you are right.

TBH I'm concerned about readability because I use ScratchPad heavily e.g.

case {a: 1}
  in String[a: 1]
    ScratchPad << :foo
else
  ScratchPad << :else
end

ScratchPad.recorded.should == [:else]

but it could be done shorter with checking case expression result like

case {a: 1}
  in String[a: 1]
    true
else
  false
end.should == false

What would you prefer?

@eregon
Copy link
Member

eregon commented Jan 16, 2020

I would use the shorter second version, it could look like this with eval:

eval(<<~RUBY).should == false
case {a: 1}
in String[a: 1]
  true
else
  false
end
RUBY

(<<~RUBY syntax highlights correctly in Atom and RubyMine at least)

@andrykonchin andrykonchin force-pushed the andrykonchin-ruby-2-7--pattern-matching branch from 2998958 to 939d629 Compare January 16, 2020 22:14
@andrykonchin andrykonchin changed the title WIP [Ruby 2.7] Add specs for pattern matching [Ruby 2.7] Add specs for pattern matching Jan 16, 2020
@andrykonchin andrykonchin force-pushed the andrykonchin-ruby-2-7--pattern-matching branch from 7abf1b8 to 9cfe5b8 Compare January 16, 2020 23:04
@andrykonchin
Copy link
Member Author

@eregon

There is only one failing spec (on MacOS) not relevant to the PR:

CVE-2019-8325 is resisted by sanitising error message components for the 'loading command' message
Example took 15.006841999999978s, which is longer than the timeout of 15.0s

Actually there was another failing spec on Windows related to sockets. After re-running it passed but another spec, mentioned above, failed.

So I would like to merge the PR as is but waiting for your approval.


hash.deconstruct_keys([:a]).should == {a: 1, b: 2}
hash.deconstruct_keys(0 ).should == {a: 1, b: 2}
hash.deconstruct_keys('' ).should == {a: 1, b: 2}
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if this is intended behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

@eregon

Looks like so - https://github.com/ruby/ruby/blob/master/hash.c#L4591-L4595

Keys passed as argument are just a hint to optimize performance. Struct#deconstruct_keys uses keys... Hash doesn't. At least for now. Hope it will be changed in the next release.

@eregon
Copy link
Member

eregon commented Jan 17, 2020

This is a great PR, thank you!

Sorry for the transient failure, I'd guess it was a particularly slow run, maybe we should just bump the timeout.

@eregon eregon merged commit a8dc71f into master Jan 17, 2020
@eregon eregon deleted the andrykonchin-ruby-2-7--pattern-matching branch January 17, 2020 12:21
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