search

IRCraziestTaxi / typeorm-linq-repository

Wraps TypeORM repository pattern and QueryBuilder using fluent, LINQ-style queries.
MIT License
185 stars 10 forks source link

OrderBy random() #218

Open tolstenko opened 3 years ago

tolstenko commented 3 years ago

Hello!

Your repo is awesome! Thanks!

Context: IQuery provides two functions regarding ordering: https://github.com/IRCraziestTaxi/typeorm-linq-repository/blob/9f1efb1e162e5203c83e78c457aed28eadf2b8a1/src/query/Query.ts#L364 and https://github.com/IRCraziestTaxi/typeorm-linq-repository/blob/9f1efb1e162e5203c83e78c457aed28eadf2b8a1/src/query/Query.ts#L375 But Probably we should have another one for Random. Or pass the ordering as a parameter.

tolstenko commented 3 years ago

here goes one suggestion. should i ask for a merge? https://github.com/sthormio/typeorm-linq-repository/commit/eec3ca77e3c445789fe51f3f7ecbccf7bbb54492

IRCraziestTaxi commented 3 years ago

Hey @tolstenko, sorry for just now getting around to looking at this.

Sure, I welcome PRs for new features! Before you create the PR, though, I have one comment on the commit you linked.

The summary for the new interface method says: Orders the query on the specified property in random order.

However, looking at the usage, orderByExpression("RANDOM()"), it doesn't take a property to sort on. In fact, I suppose that is the entire point of random sorting is that it doesn't depend on a property.

So just change the summary to Orders the query in random order. and I'll consider it good to go.

As far as the test case goes, since there are not very many songs in the test seed data (so chances of failing are higher than if the data set was larger) and since the expression depends on the DB implementation anyway, I would say you can just omit the test case.

Thanks!

IRCraziestTaxi commented 3 years ago

@tolstenko Did you get a chance to review my feedback?