-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Minor QOL improvements #46
Conversation
Thanks, I'll have a look at these. I assume your distance checks are on a text-similarity metric, otherwise you'd be using |
It works for text-similarity (using
This will work using an index |
I don't want the codebase to become too sprawling, and I'm not 100% sold on the these, so perhaps for now you could keep them in your own condition helpers module?
This is a great idea: will definitely implement.
My preferred behaviour here would be for it to be a type error to explicitly pass undefined as a Failing that, I'm not sure whether to convert In the meantime, I guess you'll continue need something like: const
breed = params?.breed,
res = await db.select('cats', breed ? { breed } : db.all).run(pgPool); or: const
breed = params?.breed,
res = await db.select('cats', { ...(breed ? { breed } : {}), otherCharacteristic: true }).run(pgPool); |
@jtfell If you can give me a PR with just the timing additions to |
Ok no worries, I'll remove them.
It looks like Sequelize throws an error in this scenario and converts I'm not in a particular rush to get any of this merged so happy to discuss further rather than just merging the timing part if thats ok with you? |
I've pondered this a bit. I don't think there's anything wrong with the throwing-on- |
This sounds reasonable to me. Do you mind if I create a new PR with only the timing change? |
@jtfell, in a fit of tidying up I ended up making this change myself in order to close the ticket. |
And it's now in 3.1.1. |
Just a few minor tweaks that have been helpful in my project (it's quite spatial and text search heavy). I'm happy to split it up into multiple PRs if you would prefer but I think its mostly uncontroversial.
<->
) related conditions (plus an ordering one that doesn't quite fit as a "condition" but maybe its okay to sneak it in alongside theor
andand
ones).resultListener
callback. This has been particularly helpful debugging query performance and will likely be useful for general logging down the trackbut this didn't work properly as the
undefined
string ended up in the generated SQL. I added in a check to just ignore the condition if its undefined.