Add Ruby 1.8.7 compatibility #43

sferik · 2013-12-31T20:44:01Z

I’d like to use Naught to replace Twitter::NullObject in the twitter gem, however, the current version of the gem still supports Ruby 1.8.7 and I don’t want to bump the major version just to add Naught.

I also took this opportunity to add other Ruby versions and implementations to the Travis CI configuration (JRuby, Rubinius, etc.).

Do you have any objection to supporting Ruby 1.8.7, at least through June 2014?

avdi · 2013-12-31T21:00:18Z

Cool! I'm actually kinda surprised it was that easy... does instance_exec work in 1.8.7?

sferik · 2013-12-31T21:10:56Z

does instance_exec work in 1.8.7?

I can’t find any instances of instance_exec in the codebase.

sferik · 2013-12-31T21:17:43Z

I'm actually kinda surprised it was that easy

It’s actually not as easy as I initially thought. Working on making the specs pass on various Ruby implementations now.

sferik · 2013-12-31T21:23:25Z

Hmmm, it looks like this will require removing BasicObject as the parent class for null objects. That seems like a pretty major change, so I wanted to check with you before I proceed with this patch.

avdi · 2014-01-01T18:58:11Z

Thoughts on adding a dependency on the backports gem instead? Or what about
defining our own ::Naught::BasicObject (like the one in ActiveSupport)
which is an alias for BasicObject if it's available?

On Tue, Dec 31, 2013 at 4:23 PM, Erik Michaels-Ober <
notifications@github.com> wrote:

Hmmm, it looks like this will require replacing BasicObject with Objectas the parent class for null objects. That seems like a pretty major
change, so I wanted to check with you before I proceed with this patch.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/43#issuecomment-31411823
.

Avdi Grimm
http://avdi.org

I only check email twice a day. to reach me sooner, go to
http://awayfind.com/avdi

sferik · 2014-01-01T19:06:29Z

Backports is cool project but it can make debugging more complicated. I prefer the idea of ::Naught::BasicObject. I’ll take that approach and continue working on this.

sferik · 2014-01-02T04:42:07Z

Okay, I’ve got the specs passing on MRI 1.9.2 through head, rbx, and jruby-head but there are still 5 spec failures on MRI 1.8.7. There seems to be something very strange happening on 1.8.7 when calling any of the conversion methods (e.g. Actual) with a block. Even though the method is passed a block, for some reason, the block parameter is nil and block_given? is false. This has me thoroughly confused. Would you mind looking into it? I’m obviously missing something.

avdi · 2014-01-03T21:24:39Z

Working on it!

avdi · 2014-01-03T21:25:46Z

While I'm waiting for 1.8.7 to install... a little buzzing at the back of my head is saying something about parsing of blocks. First thing I want to try will involve switching between curly braces and do/end.

avdi · 2014-01-03T21:41:04Z

This is super weird. I can't reproduce it with methods I define in IRB, but it's definitely happening under test.

avdi · 2014-01-03T21:48:15Z

OK, so this is pretty nuts. If I load up naught in IRB 1.8.7 and generate a null class, then include the conversions module, I can reproduce this behavior. But if I copy-and-paste the source of Actual into the IRB session and then try it, it works.

avdi · 2014-01-03T21:52:37Z

All I can think is that this is a bizarre bug in 1.8.7 that only occurs when a method is included from a module. I don't know how to work around it other than to put a conditional around the tests for the conversion functions. But that doesn't help anyone to actually use the functions in 1.8.7.

This is painful, but you might move the definitions of the conversion functions into a proc, and provide a way to instance_eval that proc in the current context rather than including the generated Conversions module. That might work in 1.8.

sferik · 2014-01-03T23:01:39Z

I agree with your conclusion that this is being caused by some bizarre bug in Ruby 1.8.7. Frankly, I’m surprised that I’ve never encountered it before and I have no recollection of reading about it around the time 1.9 was released.

For kicks, I tried requiring backports to see if that somehow magically fixed things. It did not.

This is painful, but you might move the definitions of the conversion functions into a proc, and provide a way to instance_eval that proc in the current context rather than including the generated Conversions module.

I had to read that sentence five times before I actually understood what you’re proposing and now my head hurts. I will give that a try, just as soon as my head stops hurting.

Thanks for taking the time to look into this. I was completely stumped.

avdi · 2014-01-06T14:55:06Z

I had one other thought about this, which is to have optional lower-case versions of the conversion functions. I haven't tested to see if this gets around the bug tho.

sferik · 2014-01-06T22:55:22Z

I just tested with lower-case method names and I get the same errors.

avdi · 2014-01-07T01:30:05Z

This is such a weird-ass bug.
On Jan 6, 2014 5:55 PM, "Erik Michaels-Ober" notifications@github.com
wrote:

I just tested with lower-case method names and I get the same errors.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/43#issuecomment-31696497
.

sferik · 2014-01-08T04:40:59Z

💥 I think I figured it out. This is the issue: You can’t pass a block that takes block arguments to define_method in Ruby 1.8, so it’s doing nothing here.

avdi · 2014-01-08T14:56:47Z

Argh, I really should have remembered that, it was a big deal to me when
1.9.3 changed that rule.

On Tue, Jan 7, 2014 at 11:40 PM, Erik Michaels-Ober <
notifications@github.com> wrote:

[image: 💥] I think I figured it out. Thishttp://coderrr.wordpress.com/2008/10/29/using-define_method-with-blocks-in-ruby-18/is the issue: You can’t pass a block that takes block arguments to
define_method in Ruby 1.8, so it’s doing nothing herehttps://github.com/avdi/naught/blob/3ad13c6196bf0c546d6ed7baf1fa204c417edd4d/lib/naught/null_class_builder/conversions_module.rb#L12
.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/43#issuecomment-31805470
.

Avdi Grimm
http://avdi.org

I only check email twice a day. to reach me sooner, go to
http://awayfind.com/avdi

sferik · 2014-01-08T19:52:01Z

This isn’t pretty but it solves the issue with blocks on Ruby 1.8. Unfortunately, it breaks tracing. Two steps forward, one step back. 🐸

avdi · 2014-01-08T19:55:03Z

Can you fix tracing by supplying the file and line args to eval?

On Wed, Jan 8, 2014 at 2:52 PM, Erik Michaels-Ober <notifications@github.com

wrote:

Thishttps://github.com/sferik/naught/commit/be09edbaa8938f12ecfd46616fd25f8cb36c44e7isn’t pretty but it solves the issue with blocks on Ruby 1.8.
Unfortunately, it breaks tracing. Two steps forward one step back. [image:
🐸]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/43#issuecomment-31871192
.

Avdi Grimm
http://avdi.org

I only check email twice a day. to reach me sooner, go to
http://awayfind.com/avdi

sferik · 2014-01-08T20:01:35Z

Can you fix tracing by supplying the file and line args to eval?

The first thing I tried (which requires the method_source gem on Ruby 1.8) was:

meth = mod.method(method_name)
define_method("__real__#{method_name}", &meth)
file, line = meth.source_location
class_eval <<-EOM, file, line
  def #{method_name}(*args, &block)
    __real__#{method_name}(block, *args)
  end
EOM

Unfortunately, this did not fix it. What am I doing wrong?

avdi · 2014-01-08T20:19:35Z

I was just thinking FILE and LINE, but I'm probably missing
something. I don't want to hide the eval'd code from traces.
On Jan 8, 2014 3:01 PM, "Erik Michaels-Ober" notifications@github.com
wrote:

Can you fix tracing by supplying the file and line args to eval?

The first thing I tried was (which requires the method_source gem on Ruby
1.8):

meth = mod.method(method_name)define_method("real#{method_name}", &meth)file, line = meth.source_locationclass_eval <<-EOM, file, line + 1 def #{method_name}(*args, &block) real#{method_name}(block, *args) endEOM

Unfortunately, this did not fix it but maybe I’m doing it wrong?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/43#issuecomment-31872170
.

sferik · 2014-01-08T20:32:36Z

That will set the source file and line to lib/naught/null_class_builder/conversions_module.rb:16 (i.e., where the code was evaluated). Not sure how to get access to the line/file of the caller.

sferik · 2014-01-09T11:34:07Z

There may be a more elegant way to do this but I was pretty happy just to get it working across all major Ruby versions/implementations.

Very curious to get your feedback.

avdi · 2014-01-10T22:48:47Z

lib/naught.rb

@@ -8,6 +8,4 @@ def self.build(&customization_block)
    builder.customize(&customization_block)
    builder.generate_class
  end
-  module NullObjectTag


Q: What happens when someone uses config.base = Object? Can we still identify a null object as a null object?

Ah, no. I’ll revert https://github.com/sferik/naught/commit/9d7872e86673073e2a9576c14fe7449713f54548.

Rename for consistency and clarity vs. implicit_conversions_spec.rb.

I tried doing the same thing with alias_method/undef_method but it caused errors on JRuby.

This is required to support Ruby 1.8 and earlier, which doesn't allow define_method to take a block that takes a block argument. See http://coderrr.wordpress.com/2008/10/29/using-define_method-with-blocks-in-ruby-18/

ConversionsModule was previously a class that needed to be initialized with two arguments: null_class and null_equivs. Instead, these arguments are passed via Module#included by setting a constant on the null class, including the module in the null class, and then removing the constant and undefining the included methods.

sferik · 2014-01-10T23:09:05Z

I’ve restored NullObjectTag and tests are still passing. Any other questions/objections?

avdi · 2014-01-10T23:42:32Z

Nope, I think I'm good with it!
On Jan 10, 2014 6:09 PM, "Erik Michaels-Ober" notifications@github.com
wrote:

I’ve restored NullObjectTag and tests are still passing. Any other
questions/objections?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/43#issuecomment-32076248
.

sferik · 2014-01-10T23:59:04Z

🐣 🐥 🐤 🐔 🥚

I’ve already committed to a branch in the twitter gem that replaces my custom Twitter::NullObject object with Naught. I can merge that as soon as you merge this and push a new gem. I think this will have a significant impact on the total number of users of Naught.

According to (RubyGems.org)[http://rubygems.org/gems/naught], the current version of Naught has about 1,200 downloads since November 8 or about 600 per month. The latest version of the twitter gem has about 2,400 downloads since January 6 or about 600 per day, so this should increase the number of (indirect) Naught users by a factor of 30 or so.

sferik · 2014-01-15T09:51:25Z

It’s been a few day since you said this looked good but I can’t help but noticing that it’s still not merged.

If you’re willing to make me a collaborator on the project, I’d be happy to merge it myself and help maintain the project going forward.

avdi · 2014-01-16T16:38:00Z

I'm merging this. Thanks for all your hard work! And yeah, I'd be happy to make you a collaborator.

I kind of want to think about where we go from here... one of the reasons I built this library was as a way to demonstrate a number of metaprogrammer techniques, and the 1.8.7 compat necessarily means switching to... sub-optimal techniques. I'd like to get your thoughts on how to maybe restore some of the more "ideal" metaprogramming demonstrations.

I guess the simplest thing would be to just wait until you officially sunset 1.8 support in the Twitter gem, and then do a rewrite.

Another thought I had was to isolate some of the worst kludges into their own files and maintain two versions of those files one for 1.8 and one for >1.8. Then conditionally load the correct files based on RUBY_VERSION.

Thoughts?

Add Ruby 1.8.7 compatibility

avdi · 2014-01-16T16:39:43Z

I'm also thinking of stamping this as the 1.0 release. Being used in a major gem like twitter seems like a good time to lock down the API.

sferik · 2014-01-16T17:34:17Z

The only issue with releasing this as version 1.0.0 is that locks you into supporting Ruby 1.8.7 until you release version 2.0.0. If you’re planning to remove Ruby 1.8.7 support in the relatively near future, (say, 6.months.from_now), I’d recommend releasing this as 0.1.0 or possibly 0.9.0, to signify that 1.0.0 is imminent.

On the other hand, the whole point of this patch is so that I can depend on this gem in production, and according to Semantic Versioning:

If your software is being used in production, it should probably already be 1.0.0. If you have a stable API on which users have come to depend, you should be 1.0.0. If you're worrying a lot about backwards compatibility, you should probably already be 1.0.0.

Maybe releasing version 2.0.0 in 6 months isn’t so bad after all? Firefox and Chrome have taught us that there’s no shame in being at version 26.0 or 32.0.1700.77.✡ It’s not like there’s any shortage of positive integers. In fact, quite the opposite.

✡ Chrome’s version number has been increasing by (on average) 0.01630988786 per day. If they continue at the current pace for the next 50 years, when I am 80 years old, Chrome’s version number will be 297.852803088.

Update: Replaced asterisk with Star of David, since GitHub-Flavored Markdown converts asterisks to bullet points. See also.

sferik · 2014-01-22T06:14:19Z

@avdi Any additional thoughts on shipping a new version and what number it might have?

avdi · 2014-01-25T19:15:01Z

As also tweeted: let's go with 1.0.0. I've added you as a gem owner since I keep getting sick and having snowstorms and assorted other excuses for not getting around to it :-)

sferik · 2014-01-26T14:54:05Z

Okay. I’ve reviewed all open issues and pull requests, updated the CHANGELOG, and bumped the version to 1.0.0. Here goes nuthin’… 🚢

avdi · 2014-01-26T16:23:11Z

Erik, Thanks again for all your work and patience on this. I never imagined this library would be 1.8-compatible!

sferik mentioned this pull request Jan 6, 2014

Delegate explicit conversions to nil instead of defining them explicitly #44

Merged

avdi reviewed Jan 10, 2014
View reviewed changes

sferik added 4 commits January 10, 2014 23:52

Make trailing whitespace consistent across all files

5baa7d4

Rename conversions_spec.rb to explicit_conversions_spec.rb

12b5a2c

Rename for consistency and clarity vs. implicit_conversions_spec.rb.

Run specs in random order to catch order-dependency bugs

5c454b4

Use the new expect(...).to receive syntax

cec1c89

sferik added 13 commits January 10, 2014 23:52

Use Ruby 1.8-compatible hash syntax

b5065ac

Replace stabby lambda with lambda keyword

d27002b

Replace ::BasicObject with Naught::BasicObject

8defad0

Conditionally define respond_to?/respond_to_missing?

c2e8429

I tried doing the same thing with alias_method/undef_method but it caused errors on JRuby.

Ruby 1.8, Rubinius, and JRuby do not print the indication of blocks

c891694

Fix specs on Rubinius

092aed7

Fix specs on Ruby 1.9

ba359f3

Fix specs on JRuby

ecb1073

Add pry as a development dependency

609dd93

Fix define_method with blocks in Ruby 1.8 and earlier

6560fb0

This is required to support Ruby 1.8 and earlier, which doesn't allow define_method to take a block that takes a block argument. See http://coderrr.wordpress.com/2008/10/29/using-define_method-with-blocks-in-ruby-18/

Add Ruby version guards for explicit conversion specifications

52c7547

Test against various Ruby implementations

f185e43

avdi pushed a commit that referenced this pull request Jan 16, 2014

Merge pull request #43 from sferik/ruby_1.8

1ff4858

Add Ruby 1.8.7 compatibility

avdi merged commit 1ff4858 into avdi:master Jan 16, 2014

Add Ruby 1.8.7 compatibility #43

Add Ruby 1.8.7 compatibility #43

Conversation

sferik commented Dec 31, 2013

avdi commented Dec 31, 2013

sferik commented Dec 31, 2013

sferik commented Dec 31, 2013

sferik commented Dec 31, 2013

avdi commented Jan 1, 2014

sferik commented Jan 1, 2014

sferik commented Jan 2, 2014

avdi commented Jan 3, 2014

avdi commented Jan 3, 2014

avdi commented Jan 3, 2014

avdi commented Jan 3, 2014

avdi commented Jan 3, 2014

sferik commented Jan 3, 2014

avdi commented Jan 6, 2014

sferik commented Jan 6, 2014

avdi commented Jan 7, 2014

sferik commented Jan 8, 2014

avdi commented Jan 8, 2014

sferik commented Jan 8, 2014

avdi commented Jan 8, 2014

sferik commented Jan 8, 2014

avdi commented Jan 8, 2014

sferik commented Jan 8, 2014

sferik commented Jan 9, 2014

avdi Jan 10, 2014

Choose a reason for hiding this comment

sferik Jan 10, 2014

Choose a reason for hiding this comment

sferik commented Jan 10, 2014

avdi commented Jan 10, 2014

sferik commented Jan 10, 2014

sferik commented Jan 15, 2014

avdi commented Jan 16, 2014

avdi commented Jan 16, 2014

sferik commented Jan 16, 2014

sferik commented Jan 22, 2014

avdi commented Jan 25, 2014

sferik commented Jan 26, 2014

avdi commented Jan 26, 2014