SQLite extension loading via sqlx.toml for CLI and query macros #3713
Conversation
There was a problem hiding this comment.
I'm very keen in landing this, but besides fixing the build, it does also need some sort of test to prove that it works, and ideally a compile-checked example that shows how to use it. In #3383 I realized I could take out two birds with one stone by adding some example projects showing off the new configuration. I'd suggest doing the same.
|
(I'm just now realizing the build failures are from #3383 itself so disregard that.) |
…oes not already exist
|
I'll be happy to add an example, but there's a complication: to demonstrate this functionality, it's necessary to load a SQLite extension, and that requires that the example is dependent on having that extension installed. Are you okay with having an example that has a third-party dependency which is not shipped with SQLx? If so, what is your preference for how I structure that? |
|
Our CI already downloads an extension from https://github.com/nalgeon/sqlean/ for testing: sqlx/.github/workflows/sqlx.yml Line 125 in 65229f7 One thing is that for Unix platforms, you need to show the user either how to set |
|
Is there anything further that you need from me to get this merged? |
|
Hello? Just checking in. I'd really appreciate it if this could be merged, or if you could let me know what I need to do to get it to that point. |
There was a problem hiding this comment.
I'm not a maintainer, so take my review as just from an interested party. I found this based on reading the semi-related jiff-sqlx issue at BurntSushi/jiff#272).
| /// UNSTABLE: for use with `sqlx-cli` | ||
| /// | ||
| /// Connect to the database, and instruct the nested driver to | ||
| /// read options from the sqlx.toml file as appropriate. | ||
| #[doc(hidden)] | ||
| pub fn connect_with_config(url: &str) -> BoxFuture<'static, Result<Self, Error>> |
There was a problem hiding this comment.
Would it be reasonable to make this available to apps using sqlx other than just the cli? What makes it a sqlx-cli only option? Would it make sense to instead make loading the config file default functionality (or perhaps feature gated instead of adding a new method)?
There was a problem hiding this comment.
Perhaps, but I was working on the principle of least change. The CLI is the only place I needed it. It would not be tagged as pub if that weren't a technical necessity, so marking it unstable and hiding it in the docs is the next best option.
| pub database_url: Url, | ||
| pub log_settings: LogSettings, | ||
| pub enable_config: bool, |
There was a problem hiding this comment.
Given that these are part of the public interface, they should probably all have doc comments.
There was a problem hiding this comment.
Probably, but not having a doc for enable_config is consistent with the surrounding code. Documenting all of the options would be a subject for a different pull request.
There was a problem hiding this comment.
Personally, I'd say that documenting new code is more important than being consistent with undocumented items. enable_config is effectively meaningless as a field name without additional context. Obviously we have it right now, but the docs are for people that come later.
| /// Specify extensions to load. | ||
| /// |
There was a problem hiding this comment.
Is it worth adding docs about the file path / LD_LIBRARY_PATH here similar to above?
There was a problem hiding this comment.
The reason that it's not (IMO) is the docs page is where you would generally discover information about how to configure this structure. The reference toml file may never be seen or even thought of as a possible place where there's documentation about this by someone looking for config information.
Doc formatting tweak Co-authored-by: Josh McKinney <[email protected]>
abonander
force-pushed
the
sqlx-toml
branch
2 times, most recently
from
0133038 to
5cac661
Compare
fixes issue #3330 by allowing the query checking macros to load SQLite extensions specified in sqlx.toml.
Related to that, also allows the CLI to load the SQLite extensions specified in sqlx.toml when running migrations