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

Sqlean Time extension #854

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

pedrocarlo
Copy link
Contributor

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

@@ -82,6 +82,10 @@ test-vector:
SQLITE_EXEC=$(SQLITE_EXEC) ./testing/vector.test
.PHONY: test-vector

test-time:
Copy link
Contributor

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.

Copy link
Contributor Author

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"] }
Copy link
Contributor

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 }

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants