Make SqliteDatabase write mode a kj::Maybe #1809
Conversation
There was a problem hiding this comment.
I guess that makes sense. FWIW, I had done it this way for consistency with the KJ filesystem APIs, which similarly have methods like openFile() which have read-only and writable overloads, based on whether you provide a WriteMode. But in the case of openFile, the two overloads actually return different types (ReadableFile vs File), so it really does have to be an overload in that case. Here, using Maybe is fine.
Whitespace nitpick: It looks like you have several instances of }\n else { that should be } else { instead.
src/workerd/util/sqlite.c++
Outdated
| } | ||
| else { |
There was a problem hiding this comment.
| } | |
| else { | |
| } else { |
There was a problem hiding this comment.
Fixed in d415067.
src/workerd/util/sqlite.c++
Outdated
| } | ||
| else { |
There was a problem hiding this comment.
| } | |
| else { | |
| } else { |
There was a problem hiding this comment.
Fixed in d415067.
src/workerd/util/sqlite.c++
Outdated
| } else { | ||
| SQLITE_CALL_NODB(sqlite3_open_v2(path.toString().cStr(), &db, | ||
| flags, vfs.getName().cStr())); | ||
| else { |
There was a problem hiding this comment.
Need to fix this too. (Github won't let me propose a fix here since the } on the previous line is now considered new.)
There was a problem hiding this comment.
Fixed in d415067.
That'll teach me to run |
I have a use case for SqliteDatabase where I need to parameterize whether the database is read-only or read/write. Having two constructors for these cases and no default constructor means that I cannot parameterize the read/write-mode of the SqliteDatabase in the constructor of another class. It’s much easier to use this if we can indicate we want read-only mode using a parameter. I considered using a WriteMode of 0 to mean “read-only”, but the kj::WriteMode documentation says: > (To open a file or directory read-only, do not specify a mode.) The best way to “not specify a mode” as a parameter is to make it a kj::Maybe. I made the write mode an optional parameter so that to make the new interface compatible the previous one.
justin-mp
force-pushed
the
jmp/sqlite-optional-writemode
branch
from
d415067
to
f3f1ff5
Compare
I have a use case for SqliteDatabase where I need to parameterize whether the database is read-only or read/write. Having two constructors for these cases and no default constructor means that I cannot parameterize the read/write-mode of the SqliteDatabase in the constructor of another class.
It’s much easier to use this if we can indicate we want read-only mode using a parameter. I considered using a WriteMode of 0 to mean “read-only”, but the kj::WriteMode documentation says:
The best way to “not specify a mode” as a parameter is to make it a kj::Maybe.
I made the write mode an optional parameter so that to make the new interface compatible the previous one.