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

Adding methods with a generic name to a core Ruby class (and therefore all instances of that class) is not a good idea. #25

Merged
merged 2 commits into from Sep 17, 2011

Conversation

floehopper
Copy link
Contributor

Thanks for your work on Crack. This patch fixes a nasty problem I was having when using Savon (and therefore Crack) in the same project as Mongoid. I will also try and suggest a way for Mongoid to make itself a bit more robust, but I think this change (or something similar) to Crack is a no brainer.

…e all instances of that class) is not a good idea.

Crack was defining [1] `attributes` accessor methods on the core Ruby `String` class which in turn caused a very difficult to track down bug when using Mongoid, because of this [2]. This commit changes Crack so it just defines the `attributes` accessor methods on the relevant instances of `String`.

[1] https://github.com/jnunemaker/crack/blob/master/lib/crack/xml.rb#L85
[2] https://github.com/mongoid/mongoid/blob/master/lib/mongoid/relations/builder.rb#L38
@marcusmateus
Copy link

@floehopper, great find!... a number of hours into debugging I narrowed the issue to httparty/crack, and google brought me here... you saved me more hours of debugging to find the exact line.

+1 on this... it is causing all kinds of weird very hard to track down issues in Mongoid.

@floehopper
Copy link
Contributor Author

Cool. It's just a shame you had to go through any of the debugging, given this patch was offered months ago and still hasn't been merged.

@jnunemaker jnunemaker merged commit da2fc97 into jnunemaker:master Sep 17, 2011
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