1.8.7 compatibility #5

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

Hi,

Could you please pull this 1-line change? It fixes methodfinder in Ruby 1.8.7. Thanks.

Ruby 1.8.7 doesn't have Symbol#<=>.
Convert symbols to strings before sorting.
(Works in 1.9.2 too).

BMorearty BMorearty
1.8.7 compatibility
Ruby 1.8.7 doesn't have Symbol#<=>.
Convert symbols to strings before sorting.
(Works in 1.9.2 too).
Owner

citizen428 commented Apr 23, 2011

I changed the fix to ret.sort_by(&:to_s) instead of ret.map(&:to_s).sort.map(&:intern) and credited you in the README. The new version is pushed to Rubygems.org already and I'll make sure to always run the tests with 1.9.2 and 1.8.7 in the future.

@citizen428 citizen428 closed this Apr 23, 2011

Thanks!

By the way, the reason I used 'ret.map(&:to_s).sort.map(&:intern)' instead
of 'ret.sort_by(&:to_s)' was that I think sort_by(&:to_s) will call to_s
twice for each comparison, even for nodes that have already been compared
with another node in the sort.

Brian

On Sat, Apr 23, 2011 at 4:16 PM, citizen428 <
reply@reply.github.com>wrote:

I changed the fix to ret.sort_by(&:to_s) instead of
ret.map(&:to_s).sort.map(&:intern) and credited you in the README. The new
version is pushed to Rubygems.org already and I'll make sure to always run
the tests with 1.9.2 and 1.8.7 in the future.

Reply to this email directly or view it on GitHub:
#5 (comment)

Owner

citizen428 commented Apr 24, 2011

Now I just woke up, but are two calls to to_s really worse than one to to_s and another one to intern, especially when you have to walk the array several times in the latter case? I'm not a big fan of micro-benchmarks, but I got curious:

>> syms = [*?a..?z].map(&:intern) 
#=> [:a, :b, :c, :d, :e, :f, :g, :h, :i, :j, :k, :l, :m, :n, :o, :p, :q, :r, :s, :t, :u, :v, :w, :x, :y, :z]
>> n = 10_000_000
>> Benchmark.bm do |x|
..     x.report("map-sort-map") {syms.map(&:to_s).sort.map(&:intern)}
..   x.report("sort_by") {syms.group_by(&:to_s)}
..   end 
#=> true
      user     system      total        real
map-sort-map  0.000000   0.000000   0.000000 (  0.000070)
sort_by  0.000000   0.000000   0.000000 (  0.000045)

As you can see sort_by is in fact faster, even though execution times for both are ridiculously low. But even if it were slower I'd go for sort_by since for me code clarity wins over speed unless there's a very good reason.

Thanks again for bringing up the incompatibility though! :-) I'll look into your other pull request some time later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment