Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Fix after_initialize and Base.create edge case #2236
Conversation
hmcfletch
and others
added some commits
Jul 21, 2011
guilleiguaran
reviewed
Jul 24, 2011
@@ -0,0 +1,9 @@ | ||
+class WholesaleProduct < ActiveRecord::Base |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
guilleiguaran
Jul 24, 2011
Owner
Actually the ActiveRecord maintainers would prefer that you use existing models for new tests instead of add new models
guilleiguaran
Jul 24, 2011
Owner
Actually the ActiveRecord maintainers would prefer that you use existing models for new tests instead of add new models
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
hmcfletch
Jul 24, 2011
Contributor
I poked around existing models to see if there was one I could use for this test, but I didn't see one that I could easily repurpose. I didn't want to mess with one that looked they were being used for specific test cases, which most of the ones I looked at seemed to be. I could have put it at the top of the test file, but none of the other classes up there had method declarations. It's my first submitted patch, so I just tried to not mess with things that were already in place too much.
hmcfletch
Jul 24, 2011
Contributor
I poked around existing models to see if there was one I could use for this test, but I didn't see one that I could easily repurpose. I didn't want to mess with one that looked they were being used for specific test cases, which most of the ones I looked at seemed to be. I could have put it at the top of the test file, but none of the other classes up there had method declarations. It's my first submitted patch, so I just tried to not mess with things that were already in place too much.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
cldwalker
Jul 24, 2011
Contributor
@guilleiguaran Updated tests to use existing model. Personally, I don't see it as a good testing practice to be using one model for multiple unrelated tests. Leads to unnecessary coupling and hard to find bugs
@guilleiguaran Updated tests to use existing model. Personally, I don't see it as a good testing practice to be using one model for multiple unrelated tests. Leads to unnecessary coupling and hard to find bugs |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Jul 24, 2011
Owner
I agree with you but we must try to avoid defining extra models since we already have tonnes of them and if every patch add a new model it would quickly get out of hand.
Your code and tests looks great, thanks!!!
I agree with you but we must try to avoid defining extra models since we already have tonnes of them and if every patch add a new model it would quickly get out of hand. Your code and tests looks great, thanks!!! |
added a commit
that referenced
this pull request
Jul 25, 2011
spastorino
merged commit 64affc9
into
rails:master
Jul 25, 2011
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
bboe
Aug 15, 2011
Contributor
Should a similar change also be applied to create! in validations.rb? I'm not familiar enough with the rails code to know if there are other similar places where code should be changed.
Should a similar change also be applied to create! in validations.rb? I'm not familiar enough with the rails code to know if there are other similar places where code should be changed. |
cldwalker commentedJul 24, 2011
This time rebased off master as requested in #2194