-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: add orderByRaw to loadManyByRawWhereClauseAsync #185
feat: add orderByRaw to loadManyByRawWhereClauseAsync #185
Conversation
Codecov Report
@@ Coverage Diff @@
## main #185 +/- ##
==========================================
+ Coverage 96.27% 96.29% +0.01%
==========================================
Files 80 80
Lines 2015 2023 +8
Branches 244 245 +1
==========================================
+ Hits 1940 1948 +8
Misses 75 75
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few things to add:
- In
StubDatabaseAdapter
, we should throw an exception infetchManyByFieldEqualityConjunctionInternalAsync
iforderByRaw
is specified. Same change should be applied toInMemoryDatabaseAdapter
.
One larger question: Do you think this makes more sense to limit this arg to loadManyByRawWhereClauseAsync
? Having it present in loadManyByFieldEqualityConjunctionAsync
creates a situation where we can't simulate a query in memory (see note above about adding the throws) where up until now we could. My gut says that so far we've used loadManyByRawWhereClauseAsync
as the escape hatch for needing to do special SQL things in the database adapters, so it might make more sense for this to just be available in that method (especially because we can't simulate it in the in-memory database adapters mentioned above). That being said, I think your particular use case used loadManyByFieldEqualityConjunctionAsync
so maybe not?
...ges/entity-database-adapter-knex/src/__integration-tests__/PostgresEntityIntegration-test.ts
Outdated
Show resolved
Hide resolved
packages/entity-database-adapter-knex/src/PostgresEntityDatabaseAdapter.ts
Outdated
Show resolved
Hide resolved
I think it makes sense to limit this arg to Which of the following do you think is the better approach:
|
I like this option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot better. Probably worth create a new set of types that extends the query modifiers type that includes the new param. This way the internal and external types of the library match and are named. I tried it in 3d2d189 (commit on top of yours) to be sure and it seemed to work (feel free to use all or none of that commit).
(don't worry about diff target codecov percentage) |
Ping me when you land this and I'll create a new release of these and upgrade them in www. |
Why
To achieve the sort I want for Expo
apps
(ORDER BY GREATEST(display_name, SPLIT_PART(full_name, '/', 2)) ASC
), Entity needs a raw order clause option.How
I added an
orderByRaw
property to thequerySelectionModifiers
forloadManyByRawWhereClauseAsync
and updated all of the downstream types and call sites accordingly.Test Plan
The added integration test and Typescript checks should pass.