Skip to content

model.scoped deprecated and removed in activerecord 4+ #49

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

Closed
wants to merge 1 commit into from

Conversation

visualist
Copy link

Since ActiveRecord 4.2 (Rails4) has gone and removed the scoped class method, I added a conditional return value using the recommended replacement for model.scoped, which is model.all. I made this conditional on version 4 (major # only) since I didn't want to break any legacy code that still depends on older versions.

@jrochkind
Copy link
Collaborator

Hi, this is from 2015, and unmerged with no comments.

I think it is still applicable, to getting the ActiveRecordWrapper to work under any rails 4.2+.

Can anyone who may see this tell me if this gem is still maintained? I'm not sure if I should fork it, or if there are maintainers here to review/merge PRs, or if they are open to adding new maintainers (perhaps @visualist if 4 years later he's still working in this area!), or what.

Thanks for any input.

@visualist
Copy link
Author

Oh my god this is so old! What a blast from the past. OAI is something I haven't even thought about in the past 3+ years, although I'm still working in museum circles.

As far as maintained - I got the feeling this gem wasn't even maintained back in 2015.

I'm not doing any OAI work, nothing related to it, presently. We do have an API-driven website now, so if it made sense to "rejoin" the open linked data crowd with our collections data, then I guess it could be done through Rails but not in an ActiveRecord-proper way, unless it now consumes APIs and not just databases. I'm a bit out of touch with the state-of-the-art Rails world, having re-architected and built our new site (Walker Art Center) with other technologies. Would be nice to get back into Rails, tho, I miss it.

@jrochkind
Copy link
Collaborator

Cool, I expect visualist to unsubscribe, but I'm working on getting this gem into shape and fixing this.

This is clearly a bug, but there is no existing test that exersizes it, and having trouble figuring out how to make that does, working on it.

@visualist
Copy link
Author

@jrochkind - I don't mind following, unless it gets crazy busy, which I don't expect.

@jrochkind
Copy link
Collaborator

OK, still not sure how to write a test to excersize it. But the comment says "Default to empty set, as we've tried everything else".

In modern Rails, there's an easier and better way to return a Relation for the empty set -- model.none. So that's probably the right solution.

Still wanna try to figure out a test to exersize it, but if I can't, will just commit that anyway.

@visualist
Copy link
Author

I could possibly put some mental energy into this later in the week or next week, thinking about how it might be tested. But right now I'm too busy, and this stuff is way too old for me to just remember it.

@jrochkind
Copy link
Collaborator

Eh, I'm doing good with it, but thanks @visualist ! I have a couple days, and use ruby and Rails on the regular.

I'm thinking that this behavior may not make sense. If it can't figure out how to retrieve a set, should it really just return an empty set as it's doing presently, or should it raise?

Have to figure out how to test the scenario either way. For now I guess I'll make it do what it was trying to do before (but hasn't worked in many years!).

jrochkind added a commit that referenced this pull request Aug 21, 2019
Using the newer "none" method is a great way to do what the comment said was being done, return an AR Relation that's a null set.

I'm not totally sure why this code is doing that or what's going on in the ActiveRecord::Wrapper class around 'sets' in general.

I was unable to figure out how to make a test case that exersizes this code.

Closes #49
@visualist
Copy link
Author

ok @jrochkind - good luck!

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.

2 participants