-
Notifications
You must be signed in to change notification settings - Fork 310
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
Sqlean Time extension #854
base: main
Are you sure you want to change the base?
Conversation
@@ -82,6 +82,10 @@ test-vector: | |||
SQLITE_EXEC=$(SQLITE_EXEC) ./testing/vector.test | |||
.PHONY: test-vector | |||
|
|||
test-time: |
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.
We should probably move this to the test-extensions
, in testing/extensions.py
Depending on whether or not @penberg wants this included in the default features, you would want to make sure it's building first so add it to this line:
test-extensions: limbo
cargo build --package limbo_regexp --package limbo_time
you can add a function there that (optionally) loads it first.
EDIT: nvm I looked at how much it's actually testing, and rewriting all those in python certainly isn't necessary. So it just depends on whether it needs to be loaded first.
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.
This is one thing, I was a bit uncertain on how to do properly. I really wanted to test using tcl like we do with other tests, but to do so it had to be bundled together with limbo. So I added it to default, so I could just make limbo
and then open the cli and test my changes. Maybe we could have another make command to bundle particular extensions together?
|
||
[dependencies] | ||
chrono = "0.4.39" | ||
limbo_ext = { path = "../core", features = ["static"] } |
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.
In order for the extensions to work when loaded at runtime(without "static"), we need to add the following
[target.'cfg(not(target_family = "wasm"))'.dependencies]
mimalloc = { version = "*", default-features = false }
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.
Yeah, I read it a couple of days ago in the extension core readme, but totally forgot to add it here.
https://github.com/tursodatabase/limbo/tree/main/extensions/core.
Will correct this
}; | ||
} | ||
|
||
macro_rules! tri { |
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.
love these! 🎉 I'm thinking maybe these should be added to and exported from extensions/core
. Might have to change the names to be a bit more clear though.
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.
Awesome thanks! I wanted the tri macro to be named try! but it would conflict with rust's now deprecated macro. Maybe we could rename ok_tri to opt_try? And value_tri to value_check? I am a bit bad at naming macros, unfortunately.
This PR implements a sqlean time compatible extension. I would appreciate some help to review my code and see if there are ways to enhance it. Also, if there is some edge case, I have missed please tell me.
https://github.com/nalgeon/sqlean/blob/main/docs/time.md