Skip to content

Commit

Permalink
use map and Hash[] instead of inject({})
Browse files Browse the repository at this point in the history
  • Loading branch information
@sferik
wtn authored and sferik committed Mar 7, 2011
1 parent 9242532 commit a2b0b51
Showing 1 changed file with 1 addition and 3 deletions.
4 changes: 1 addition & 3 deletions lib/twitter/configuration.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,7 @@ def configure

# Create a hash of options and their values
def options
VALID_OPTIONS_KEYS.inject({}) do |option, key|
option.merge!(key => send(key))
end
Hash[VALID_OPTIONS_KEYS.map {|key| [key, send(key)] }]
end

# Reset all configuration options to defaults
Expand Down

8 comments on commit a2b0b51

@sferik
Copy link
Owner

@sferik sferik commented on a2b0b51 Apr 18, 2011

Choose a reason for hiding this comment

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

Apparently this doesn't work in Ruby 1.8.6 (See #157)

@laserlemon
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't even get the tests to run in 1.8.6. So is #157 still important?

@wtn
Copy link
Contributor

@wtn wtn commented on a2b0b51 Apr 19, 2011

Choose a reason for hiding this comment

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

You should totally revert it—the patch is faster in 1.8.7 but slower in 1.9

@jnunemaker
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just drop support for 1.8.6, but the regression in 1.9 (if for realz) seems like an issue. Maybe just do the old/boring new hash and assign keys rather than the one liner.

@sferik
Copy link
Owner

@sferik sferik commented on a2b0b51 Apr 19, 2011

Choose a reason for hiding this comment

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

Any performance difference between the two versions is negligible. I am highly skeptical that this code is causing any bottlenecks.

Sure, we could drop support for Ruby 1.8.6, but issue #157 was created by a real person who is still using 1.8.6 and wants to use this gem and, apparently, reverting this change allows him to do so.

We, the people commenting on this commit, are all "edge riders" when it comes to upgrading software, but many newer users aren't using rvm and don't know how to upgrade Ruby on their system, which typically requires building from source. Keep in mind, Ruby 1.8.6 was the version that shipped with Mac OS X Leopard, so any non-upgrader with a two-year-old Mac is likely running 1.8.6 in development. I'm also pretty sure that 1.8.6 was the Ruby version that shipped with a number Linux distributions until recently.

If supporting 1.8.6 becomes a maintenance nightmare, I'd be the first to say we should drop it, but as long as the problems can be easily fixed and don't impose a significant burden on the existing codebase, I say we should make our best effort to support it.

@wtn
Copy link
Contributor

@wtn wtn commented on a2b0b51 Apr 20, 2011

Choose a reason for hiding this comment

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

I misspoke when I said this commit makes the method slower under 1.9. This commit should be faster under 1.8.7 or 1.9.

In my 1.9 benchmark, using inject and merge takes about 50% longer than map and Hash[]. Using []= in the inject loop is about 10% faster than map and Hash[] (thanks to optimizations to inject in 1.9).

@laserlemon
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still can't fire up the ol' 1.8.6 tests but I can't imagine there being much wrong with plain vanilla each and []=:

jnunemaker/twitter@b285705

@wtn
Copy link
Contributor

@wtn wtn commented on a2b0b51 Apr 22, 2011

Choose a reason for hiding this comment

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

Laserlemon is correct—each and []= is most efficient in every Ruby implementation. It is also very clear.
That's the pattern used in Rails.
Regarding 1.8.6, I was able to run the test suite successfully. Try modifying the gem dependencies to specify exactly rack version 1.1.0 (and not a newer version).

Please sign in to comment.