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

[SQLite Dialect] Properly translate .ignore() #916

Open
BitPhinix opened this issue Mar 18, 2024 · 3 comments · May be fixed by #976
Open

[SQLite Dialect] Properly translate .ignore() #916

BitPhinix opened this issue Mar 18, 2024 · 3 comments · May be fixed by #976
Labels
api Related to library's API enhancement New feature or request good first issue Good for newcomers greenlit Ready for implementation sqlite Related to sqlite

Comments

@BitPhinix
Copy link

Currently .ignore() using the SQLite dialect gets translated into INSERT IGNORE INTO ....

Screenshot 2024-03-18 at 14 56 02

Which doesn't match the SQLite expected INSERT OR IGNORE INTO (see here)

Screenshot 2024-03-18 at 14 57 55
@BitPhinix BitPhinix changed the title [SQLLite Dialect] Properly translate .ignore() [SQLite Dialect] Properly translate .ignore() Mar 18, 2024
@koskimas koskimas added greenlit Ready for implementation good first issue Good for newcomers sqlite Related to sqlite labels Mar 19, 2024
@igalklebanov
Copy link
Member

igalklebanov commented Mar 19, 2024

Hey 👋

Nice catch!

@koskimas I wonder what the solution should be.

The low hangin' fruit is just adding an override in SqliteQueryCompiler that appends an or. The purist in me screams "But it's not WhatYouSeeIsWhatYouGet!!!".

WYSIWYG alternatives are:

db.insertInto('person').or('abort')
db.insertInto('person').or('fail')
db.insertInto('person').or('ignore')
db.insertInto('person').or('replace')
db.insertInto('person').or('rollback')
db.insertInto('person').orAbort()
db.insertInto('person').orFail()
db.insertInto('person').orIgnore()
db.insertInto('person').orReplace()
db.insertInto('person').orRollback()

But having or('ignore') or orIgnore() AND ignore() might be confusing.

@igalklebanov igalklebanov added the bug Something isn't working label Mar 19, 2024
@koskimas
Copy link
Member

koskimas commented Mar 19, 2024

It's not wysiwyg but it's so close that I could live with it.

But supporting all those other or thingies is a good idea, and in that case the separate method(s) is better. I'd go with separate methods.

We might still override ignore to work as well to avoid mistakes?

@igalklebanov
Copy link
Member

We might still override ignore to work as well to avoid mistakes?

I definitely think that's the nice thing to do.

@igalklebanov igalklebanov added enhancement New feature or request api Related to library's API labels Mar 19, 2024
@vincentiusvin vincentiusvin linked a pull request May 1, 2024 that will close this issue
@igalklebanov igalklebanov removed the bug Something isn't working label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request good first issue Good for newcomers greenlit Ready for implementation sqlite Related to sqlite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants