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

Missing constructs #73

Open
octavpo opened this issue Aug 30, 2017 · 8 comments
Open

Missing constructs #73

octavpo opened this issue Aug 30, 2017 · 8 comments

Comments

@octavpo
Copy link

octavpo commented Aug 30, 2017

I'm trying to translate from squeel, I couldn't find the equivalent of a few constructs: reorder, preload, includes, eager_load. Have they been implemented? Thanks.

@rzane
Copy link
Owner

rzane commented Aug 30, 2017

Includes, preload, and eager_load were intentionally excluded. I don't see how baby squeel would actually add any value to those methods, since they just take symbols anyway.

But, you're right about reorder. I definitely missed that one.

@octavpo
Copy link
Author

octavpo commented Aug 30, 2017

Well, squeel's preload and co. would also take stubs as arguments, so I could write: preload{assignments.students.user_role}. Not a big difference, but a little shorter and more uniform with the rest of the constructs.

@rzane
Copy link
Owner

rzane commented Sep 1, 2017

I'm going to add this to compatibility mode, but, I don't think there needs to be a DSL just for the sake of having a DSL.

@rzane rzane closed this as completed in #75 Sep 1, 2017
@octavpo
Copy link
Author

octavpo commented Sep 2, 2017

What about the reorder? And btw there's also a rewhere. Thanks.

@rzane rzane reopened this Sep 2, 2017
@rzane
Copy link
Owner

rzane commented Sep 2, 2017

Whoops! Forgot about that one! I'll add it.

@octavpo
Copy link
Author

octavpo commented Sep 11, 2017

I don't understand the reasoning for not adding includes and co. to the DSL. You're basically sending people back to AR. Or to the compatibility mode that you advise against. The whole purpose of squeel and baby_squeel is to make things easier/nicer than AR, after all there's nothing we can't do with just AR itself. Sure if you compare preload{problems} with preload(:problems), there's not much advantage for the first one. But in one place I have in squeel:

preload{[problem_sets_problems.problem_summaries.problem_state, assignments_problem_sets, student_assignments, categories_problem_sets, problem_sets_skills.skill, assets_problem_sets]}

which translates in AR to:

preload(:assignments_problem_sets, :student_assignments, :categories_problem_sets, :assets_problem_sets, problem_sets_problems: {problem_summaries: :problem_state}, problem_sets_skills: :skill)

I think the AR equivalent is definitely less clear, and I can't even keep the original order, I get a syntax error (sure order doesn't matter for the program, but it can matter for the programmer).

Besides that, there's a matter of language consistency. In a chain of baby_squeel calls using one syntax I have to go back and use AR syntax just because some constructs are missing. That's ugly. It would be great if baby_squeel provided a full set of equivalents to all AR query functions. Thanks.

@rzane
Copy link
Owner

rzane commented Sep 11, 2017

I don't think there's anything wrong with encouraging people to use AR for includes. Using baby_squeel doesn't get you anything except a fancy syntax. I think the purpose of baby_squeel is to give you a more powerful way to generate SQL queries. It's not just "for pretty looks".

That being said, this feature isn't that hard to support, so I'll add it.

@octavpo
Copy link
Author

octavpo commented Sep 13, 2017

Sounds great, thanks. And since we're at it, it would also be nice to have back the operator aliases (>>, <<) to in and not_in. Thanks again.

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

No branches or pull requests

2 participants