feat: add support for "limit 0" #2255
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndriiSherman
approved these changes
Jul 19, 2024
|
Thanks a lot! It would be awesome if you could add tests for each case. If you need guidance on how to do it, please ping me |
|
@AndriiSherman This is my first contribution to Drizzle and I have not made tests before, so if you could guide me that would be great. |
|
Sure! We have an Then you can add tests for MySQL and SQLite. To run tests, navigate to the |
|
Thanks! I setup a Github Codespace, and ran I seem to have all the TS types, so the install looks fine. It's failing before I even added any tests I imagine, something like this would be a sufficient test, though, right?? test('limit 0', async (ctx) => {
const { db } = ctx.pg;
await db.insert(usersTable).values({ name: 'John' });
const users = await db
.select()
.from(usersTable)
.limit(0);
expect(users).toEqual([]);
}); |
|
@AndriiSherman I added the tests, but as seen above, they all fail. Even before I added my tests. |
I need to fix main branch for tests from forks. I'll do it today and rerun your tests |
|
@sillvva, yes, the tests you've sent should be enough for this positive case. You can also add a test with negative numbers in the limit |
|
@sillvva, tests should be fixed on our side. It seems like it's a dprint issue now. Could you please format all the files that you changed so they can pass dprint checks? |
|
@AndriiSherman I formatted it with dprint and pulled your changes, but am still getting mostly failed tests. |
Please add a .env file to the integration-tests folder with the variable SKIP_EXTERNAL_DB_TESTS=true. I'll update the CONTRIBUTION guide with this information |
|
But the tests seem to pass, so this is mostly for you if you want to make more PRs. Your commits are not signed, so please sign them in your next PRs. I'll accept this PR without signed commits, but for future ones, please sign them - how to sign commits |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2011
MySQL, PostgreSQL, and SQLite all support LIMIT 0 as a way to quickly return an empty dataset. Negative numbers are not supported.
It can be useful for testing queries, but in Drizzle's case, it can also be a simpler way to get conditional nested data without messing up the Typescript return type as demonstrated in #2011
The changes proposed ensure placeholders (object) and numbers >= 0 are still supported. If a developer tries to use a negative number, it would exclude the limit rather than throw an error.