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 2.2 language specs #3364

Closed
wants to merge 35 commits into from
Closed

Conversation

ruipserra
Copy link
Contributor

This PR adds specs for the the following language changes in ruby 2.2 (see NEWS file):

  • nil/true/false objects are frozen. [Feature #8923]
  • Hash literal: Symbol key followed by a colon can be quoted. [Feature #4276]
  • fixed a very longstanding bug that an optional argument was not
    accessible in its default value expression. [Bug #9593]

I'll go over some points about the commits:

  • I wasn't sure where to put the nil/true/false specs. I considered creating nil_spec.rb, true_spec.rb and false_spec.rb in spec/ruby/language with a single example for frozen?, but for the sake of consistency with the existing specs I ended up putting them in kernel/frozen_spec.rb. Was this the best choice?
  • The expectation for the new quoted symbol syntax uses an eval because otherwise it would break the whole file (the parser would throw a syntax error and no examples would be run).

Thank you for your time.

ruipserra and others added 30 commits March 13, 2015 00:07
In Ruby 2.2+, we have #receiver, so it makes sense to use consistent names
here and and get rid of #self and #self= in favor of #receiver and #receiver=
These were lost by accident while resolving merge conflicts between the master
branch and the 2.2 branch.
In order not to break the whole hash_spec.rb, the new syntax is eval'd.
@yorickpeterse
Copy link
Contributor

[...] consistency with the existing specs I ended up putting them in kernel/frozen_spec.rb. Was this the best choice?

Looks fine to me.

The expectation for the new quoted symbol syntax uses an eval because otherwise it would break the whole file (the parser would throw a syntax error and no examples would be run).

We'll probably need to re-generate the parser first, then we should be able to remove the use of eval.

@ruipserra
Copy link
Contributor Author

Hey @yorickpeterse, thanks for the feedback. I'll probably write a few more language specs in the next couple of days. I'll update the PR when I do.

@ruipserra
Copy link
Contributor Author

Hi everyone,

  • Added block and lambda literal specs for MRI issue #9593 (optional argument accesible in its default value expression)
  • Updated hash_spec according to MRI issue #10315 (ChangeLog)
  • Removed assertion checking that U+180E matches /[[:blank:]]/, as it's no longer considered a whitespace character

At this point I still see 3 failed specs with MRI 2.2.1 in spec/ruby/language, but I suspect one or two of those to be MRI bugs. I opened this issue, and I'm still waiting for feedback.

@yorickpeterse
Copy link
Contributor

@ruipserra Any feedback on the Redmine ticket you submitted? Sometimes it can be a bit hard to get a hold of Matz & co, in that case replying to the ticket (and specifically mentioning somebody like Koichi or Nobu) sometimes can do the trick.

Unless this particular Redmine ticket is blocking I'll take a look at this PR this weekend.

@tak1n
Copy link
Member

tak1n commented May 28, 2015

Fail, I didn't see this PR until now.
Many of these commits are already on the 2.2 branch.
How should we proceed?

@yorickpeterse
Copy link
Contributor

I think this needs a rebase as there are a bunch of commits that aren't yet on our 2.2 branch.

@brixen
Copy link
Member

brixen commented May 29, 2015

The easiest way to do this is probably cherry-pick the commits from here to the 2.2 branch so we can close this and avoid the confusion. If @ruipserra, @yorickpeterse, or @tak1n can't do this, I'll do it in a day or two.

@yorickpeterse
Copy link
Contributor

Commits rebased into the 2.2 branch, thanks!

@tak1n
Copy link
Member

tak1n commented May 29, 2015

Awesome thanks :)

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.

6 participants