Yay! My first accepted patch!
My first patch to Rails was accepted today. I was bored yesterday, and talking with Hampton, who was working on a really cool patch himself (which I’m sure he will explain in good time). We both noticed that four or five of the unit tests for ActiveRecord::Calculations were bombing out, and naturally did what Rails hackers do:
Hampton: Weird…. a fresh checkout is causing test errors…
Coda: On calculations?
Hampton: Something broke between 4485 and 4491
Hampton: However, the last to change to AR happened at 4484
Coda: Yeah, it’s broken. I’m looking into why.
Hampton: RACE!
81 seconds later…
Hampton: Found it!
Hampton totally schooled me. I had just gotten my debug code up and running when, just 81 seconds after the green light, Hampton found the problem. That’s why he gets mentioned in keynote addresses, I guess.
Yes, the newly added Enumerable#sum was mucking up ActiveRecord::AssociationProxy’s ability to pass off the sum method to ActiveRecord::Calculations#sum. Fixing it, however, was a hell of a lot harder, since neither of us had played around with ActiveRecord::AssociationProxy. Thankfully, I figured out that sum needed to be added to ActiveRecord::AssociationCollection, which is what’s actually used when ActiveRecord::Base#find returns an array of records instead of Array. I made the fix, verified that it passed all the unit tests, and submitted it. Nicholas Seckar commited the patch today.
So I finally got to give back to Rails, even if it was just a four or five lines of code. That makes me really happy. Rails is an open-source project, and it’s important that the community give back to it as much as possible. The greatest gift you can give a project like Rails is a patch for an outstanding ticket which conforms to the style guidelines, passes existing unit tests, and has new unit tests for any new code.
I hope they get around to my patch for eagerly-loaded associations not respecting the :order option. That’d be awesome.
June 25th, 2006 at 9:54pm
Way to go! It is a great feeling when you can help out in an “official” sort of way.
I do wonder why the original code could have been added if it broke exisiting tests. I guess I thought everything committed had to first go through the full regression suite… I guess not. :-)
June 25th, 2006 at 10:43pm
DHH probably checked the ActiveSupport tests, then committed it without checking the ActiveRecord tests. Definitely an argument for a better unit test structure, like
rake testfor the Rails source.June 26th, 2006 at 8:55am
Good work - clean code!
[…] A very accosting layout and a interesting discussion topic, do you provide any Web-based services to universities or students. […]
Greetings Milos
June 26th, 2006 at 6:52pm
Actually, the sum function was added to ActiveSupport by Nicholas Seckar. Though, I don’t really blame him. When you add something (seemingly non-destructively) to ActiveSupport, it may not be obvious to run the AR unit tests.
Though, I think that they will be more careful next time.
Congradulations and welcome to the club!
-hampton.
June 26th, 2006 at 7:08pm
I’m pretty sure it was DHH. Look at the checkin for 4489. Username, david. Initials, DHH. If it isn’t DHH, they need to sit down a fucking hash out their usernames a bit better. ;-) (Nicholas (aka ulysses) checked in my patch in 4493.)
But yeah, I don’t blame him either at all. I do think they should spend an hour or two getting continuous integration set up and a Rails-wide rake test task.
June 27th, 2006 at 9:06am
Dangit!
Your right. I was misreading for r4491 not r4489.
I think you’re on to something about integrated testing.
-hampton.
June 27th, 2006 at 2:36pm
Well done. Feels good, eh? I had a couple of micro patches accepted a while ago, and now I’ve got a taste for it. Those Rails guys, they’re like pushers.
February 21st, 2007 at 4:06am
that´s always a good feeling :)
i love those stories because if you read them you fell the thing he wants to say :P
just kepp up the great work and keep on patching