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

Add random_order method to query objects #765

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

wout
Copy link
Contributor

@wout wout commented Nov 19, 2021

Here's a proposal for the random_order implementation (#755).

Initially, I tried to add a :random option to Avram::OrderBy but a few things were bad about that approach:

  • too many if statements to switch between order by random and order by column clauses
  • unnecessary attributes for the random option
  • unused column argument in the initializer, requiring the column attribute to be nilable
  • clunky internal API (e.g. Avram::OrderBy.new(nil, :random))

That's why I decided to create an abstract class called Avram::OrderByClause and split out the two different order methods into two subclasses called Avram::OrderBy and Avram::OrderByRandom. The Avram::OrderBy class could have been renamed to Avram::OrderByColumn for consistency, but by keeping the old name, we're not breaking anything.

Having multiple order by classes also will make it easier to add more order methods later on. Like ordering by an array for example, which is something I've needed in the past. For example, we could also add an Avram::OrderByArray class, generating the following clause:

ORDER BY array_position('{5,1,4,3,2}'::int[], item.id::int)

Which could look like:

Avram::OrderByArray.new(:id, [5,1,4,3,2], :nulls_last)

src/avram/query_builder.cr Outdated Show resolved Hide resolved
@jwoertink
Copy link
Member

wow! This is a lot more code than I was thinking 😂 This is pretty great though. It does feel like this will allow things to be more extensible for the future

@wout
Copy link
Contributor Author

wout commented Nov 19, 2021

Yeah, I didn't expect to add this amount of code either. But you know how those things go...

@wout
Copy link
Contributor Author

wout commented Nov 19, 2021

You're right, re-using the column attribute is much more elegant.

@wout wout marked this pull request as ready for review November 26, 2021 14:28
@wout wout changed the title WIP: Add random_order method to query objects Add random_order method to query objects Dec 1, 2021
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jwoertink jwoertink merged commit e377697 into luckyframework:master Dec 10, 2021
@wout
Copy link
Contributor Author

wout commented Dec 10, 2021

Shall I update the docs as well?

@jwoertink
Copy link
Member

We should at least open an issue to update docs. It won't go out until next release

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