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

Use AssociationReflection#klass rather than constantizing the #class_name #21

Closed
wants to merge 1 commit into from
Closed

Conversation

fw42
Copy link
Contributor

@fw42 fw42 commented Sep 22, 2014

This fixes a small bug that appears when using namespaced model names.. Something like this:

class Foo < ActiveRecord::Base
  has_one :bar
end

in app/models/foo.rb and

class Foo::Bar < ActiveRecord::Base
end

in app/models/foo/bar.rb (with table name foo_bars).

That will work just fine in Rails, but the current code on master will raise a NameError exception because it tries to constantize "Bar" (rather than "Foo::Bar"). This PR fixes that by using the AssociationReflection#klass rather than #constantize on #class_name, which is already smart enough to find the right class.

I didn't add any tests yet because the tests (on master) don't pass for me locally.. Are they supposed to?

@trptcolin, please take a quick look

@fw42
Copy link
Contributor Author

fw42 commented Sep 22, 2014

Tests are failing for me because your gemspec doesn't lock to a specific rspec version and you are using things that are deprecated in the most recent version of rspec...

@fw42
Copy link
Contributor Author

fw42 commented Sep 22, 2014

Tests pass after rebasing on #22 and after changing a few mocks to allow #klass rather than #class_name.

If you decide to merge #22, I will rebase this branch on master and add some specific tests for my bugfix.

@fw42
Copy link
Contributor Author

fw42 commented Sep 23, 2014

rebased and fixed broken tests. passing now locally. will add some new tests in a bit.

@trptcolin
Copy link
Owner

Great, thanks! One favor to ask: can you roll the "fix tests" commit up into the first commit (via git rebase -i or whatever)?

@fw42
Copy link
Contributor Author

fw42 commented Sep 23, 2014

Squashed the commits... Not sure how to reasonably add tests for the new changes with all the mocking (the tests don't actually test ActiveRecord classes but just doubles). Suggestions? Feel free to just merge if you think new tests are not necessary.

@trptcolin
Copy link
Owner

Yeah, every time I get back into this code I feel like it'd be better for most of these tests to be higher-level ones that actually use AR, maybe just using SQLite. I feel like I had a branch laying around somewhere, but I'm not finding anything.

If you have the bandwidth to add some that'd be amazing. Otherwise just updating the doubles is fine, and I can dig up a couple old projects over the next couple days to try it out manually (not using much Ruby at work right now).

@fw42
Copy link
Contributor Author

fw42 commented Jan 5, 2015

Sorry, totally forgot about this. I have no time right now to properly fix the tests :-(

Do you want to merge it anyway?

@fw42
Copy link
Contributor Author

fw42 commented Feb 2, 2016

Thanks for merging!

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

2 participants