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

Fix for Rubinius #1281

Merged
merged 1 commit into from
Aug 16, 2014
Merged

Fix for Rubinius #1281

merged 1 commit into from
Aug 16, 2014

Conversation

JoshCheek
Copy link
Contributor

Problem

Currently, CI is failing b/c singleton_class? was added to MRI2.1 but not Rbx. e.g. https://travis-ci.org/pry/pry/builds/32133642

I did not look into why it is suddenly failing now. For quite a while, Rbx builds were erroring out (e.g. https://travis-ci.org/pry/pry/jobs/30421343) They are no longer erroring, but are now failing, for the above reason.

Context

Rbx has recently (11 days ago, 30 July 2014) received this method rubinius/rubinius@2e71722

But the functionality has been available since April 2011 rubinius/rubinius@4310f6b

Solution

Fix the failures by falling back to the older functionality if we are on Rubinius, and the new method is unavailable.

Problem
-------

Currently, CI is failing b/c singleton_class? was added to MRI2.1 but not Rbx.
e.g. https://travis-ci.org/pry/pry/builds/32133642

However, I did not look into why it is suddenly failing now.
For quite a while, Rbx builds were erroring out (e.g.  https://travis-ci.org/pry/pry/jobs/30421343)
They are no longer failing, but are now erroring, for the above reason.

Context
-------

Rbx has recently (11 days ago, 30 July 2014) received this method
rubinius/rubinius@2e71722

But the functionality has been available since April 2011
rubinius/rubinius@4310f6b

Solution
--------

Fix the failures by falling back to the older functionality if we are on Rubinius,
and the new method is unavailable.
JoshCheek added a commit to JoshCheek/pry that referenced this pull request Aug 10, 2014
Removes Bacon and Mocha

Mostly this went smoothly. There were a few errors that I fixed along
the way, e.g. tests that were failing but for various reasons still
passed. Should have documented them, but didn't think about it until
very near the end. But generaly, I remember 2 reasons this would happen:
`lambda { raise "omg" }.should.raise(RuntimeError, /not-omg/)` will pass
because the second argument is ignored by Bacon. And `1.should == 2`
will return false instead of raising an error when it is not in an it
block (e.g. if stuck in a describe block, that would just return false)

The only one that I felt unsure about was spec/helpers/table_spec.rb
`Pry::Helpers.tablify_or_one_line('head', %w(ing)).should == 'head: ing'`
This is wrong, but was not failing because it was in a describe block
instead of an it block.  In reality, it returns `"head: ing\n"`,
I updated the test to reflect this, though I don't know for sure
this is the right thing to do

This will fail on master until pry#1281 is merged.
This makes pry#1278 unnecessary.
JoshCheek added a commit to JoshCheek/pry that referenced this pull request Aug 10, 2014
Removes Bacon and Mocha

Reasoning explained in this comment: pry#277 (comment)

Mostly this went smoothly. There were a few errors that I fixed along
the way, e.g. tests that were failing but for various reasons still
passed. Should have documented them, but didn't think about it until
very near the end. But generaly, I remember 2 reasons this would happen:
`lambda { raise "omg" }.should.raise(RuntimeError, /not-omg/)` will pass
because the second argument is ignored by Bacon. And `1.should == 2`
will return false instead of raising an error when it is not in an it
block (e.g. if stuck in a describe block, that would just return false)

The only one that I felt unsure about was spec/helpers/table_spec.rb
`Pry::Helpers.tablify_or_one_line('head', %w(ing)).should == 'head: ing'`
This is wrong, but was not failing because it was in a describe block
instead of an it block.  In reality, it returns `"head: ing\n"`,
I updated the test to reflect this, though I don't know for sure
this is the right thing to do

This will fail on master until pry#1281 is merged.
This makes pry#1278 unnecessary.
@ConradIrwin ConradIrwin merged commit d141301 into pry:master Aug 16, 2014
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.

None yet

2 participants