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 ruby 1.8 bug with ActiveSupport::OrderedHash #1359

Merged
merged 1 commit into from May 27, 2011
Merged

Fix for ruby 1.8 bug with ActiveSupport::OrderedHash #1359

merged 1 commit into from May 27, 2011

Conversation

AndrewRadev
Copy link
Contributor

An example of the problem can be seen in this gist: https://gist.github.com/994919.

Basically, ActiveSupport::OrderedHash behaves differently than the built-in Hash when yielding to a block that accepts its parameters with a splat. This is only an issue on ruby 1.8 because 1.9 uses the built-in Hash at all times. It's a very minor inconsistency, but it causes some problems with pp, for example.

Tested with rvm-installed ruby 1.8.7-p330, on Mac OS X 10.6.7. At least on this machine, tests are all running green.

ActiveSupport::OrderedHash did not behave identically to Hash when given
a block with a splat.
josevalim added a commit that referenced this pull request May 27, 2011
Fix for ruby 1.8 bug with ActiveSupport::OrderedHash
@josevalim josevalim merged commit 4ae8381 into rails:master May 27, 2011
@pixeltrix
Copy link
Contributor

This now makes each_pair different for 1.8 and 1.9 - is this what we want? Anybody using each_pair on ordered hashes is now going to have implement a version check - developers writing apps probably won't be affected that much but what about gem/plugin authors using Active Support? If you've gone to the trouble of using an AS::OrderedHash in a gem it's likely you'll want the same behaviour on 1.8 and 1.9.

@josevalim
Copy link
Contributor

@pixeltrix Does each_pair behaves differently on 1.8 and 1.9 for Hash? If so, it is ok for us to be differently here, if not, we should revert this.

@pixeltrix
Copy link
Contributor

Yes it behaves differently - my concern is that a gem/plugin may be using AS:OrderedHash for consistent behavior between 1.8 and 1.9. It at least needs to be noted in the CHANGLOG. :-)

@josevalim
Copy link
Contributor

Agreed.

@AndrewRadev
Copy link
Contributor Author

I added an entry to the CHANGELOG on my fork: https://github.com/AndrewRadev/rails/commit/07a353dde1aab7eabf082f4e07cf41f9dc3b2664. I can't tell if this information is enough, though. Should I explain it in a bit more detail?

Um, I also don't actually know how to add it to this pull request, or even if it's possible considering it's closed :). Sorry, I'm afraid I'm not very experienced with pull requests.

@josevalim
Copy link
Contributor

@AndrewRadev it looks good. However this fix is not going into 3-1-stable. So you just need to put it above. As per the pull request, you have to send a new one and then just link here. Thanks for your work!

@AndrewRadev
Copy link
Contributor Author

this fix is not going into 3-1-stable

Sorry about that, moved it under 3.2.0. Here's the link to the pull request, hope it's okay now: https://github.com/rails/rails/pull/1378/commits

Thanks for your work!

Glad I could help :).

josevalim added a commit that referenced this pull request May 28, 2011
Add changelog entry for pull request #1359
jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011
…methods, and reducing routes creation by 50% [rails#1359 state:committed]

Signed-off-by: David Heinemeier Hansson <david@loudthinking.com>
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

3 participants