edgedb / edgedb-rust

The official Rust binding for EdgeDB
https://edgedb.com
Apache License 2.0
215 stars 26 forks source link

macro usage ergonomics #123

Closed izirku closed 1 year ago

izirku commented 2 years ago

Summary of changes

supersedes PR #122

P.S. I ran the available tests again, all passed.

tailhook commented 2 years ago

Thanks! Code looks good but I've just realized that there is an issue: edgedb-client is just an async-std client, and we are working on tokio client and webassembly client. Currently, they are in different crates. And while tokio and async-std could be merged into a single implementation by using "feature" to switch (I'm not sure this is the best course of action, though). WebAssembly is currently blocking client and very different from others, although eventually we want to merge it too (but we need async wasm to be standardized better before this can be a reality).

Sorry, that I've realized this only now, but I have to think about it more, so that this doesn't block tokio and wasm work.

izirku commented 2 years ago

@tailhook , completely understandable! I noticed that there is some work on Tokio based client off of wasm branch, looking forward! AFAIK Tokio is used more, especially with the Axum and Tower stack gaining traction. Right, organizing codebase that deals with multiple async and tls implementations takes some thinking! This PR can wait or I can look at creating one for wasm branch as well, to mirror these changes. Let me know!

izirku commented 2 years ago

@tailhook , btw, I tried to merge this PR to wasm branch locally, no conflicts. Was able to cargo build after adding "sync" feature to tokio dependency

tailhook commented 2 years ago

@tailhook , btw, I tried to merge this PR to wasm branch locally, no conflicts. Was able to cargo build after adding "sync" feature to tokio dependency

The problem is not merge conflict per se. The problem is that in users's code edgedb_client is not importable (the client is at edgedb_sdk::client, which is not public yet, but will be shortly).

I mean ::edgedb_protocol::something works everywhere for now, but ::edgedb_client::protocol::something will not be importable until we are able to merge async-std, tokio and wasm (which is blocking) under the same crate name.

tailhook commented 2 years ago

One option would be to make a edgedb crate that is basically set of reexports

use edgedb_protocol as protocol;
use edgedb_errors as errors;
use edgedb_query_builder as qb;

#[cfg(feature="tokio")]
use edgedb_tokio as tokio;
#[cfg(feature="async-std")]
use edgedb_astd as astd;

#[cfg(feature="blocking-async-std")]
use edgedb_astd::blocking as blocking;
#[cfg(feature="blocking-wasm")]
use edgedb_sdk::client as blocking;

#[cfg(feature="default-tokio")]
use edgedb_tokio::Client as Client;
izirku commented 2 years ago

@tailhook, that's a big decision, I would defer to you & the core team :)

Overall should work, however, there may be some problems rendering docs.rs/edgedb documentation, this would need to be tested

tailhook commented 2 years ago

Note to myself: whenever we reexport protocol we should re-export edgedb_protocol::model too for convenience.

tailhook commented 1 year ago

This PR is about edgedb-client which was removed from this repository some time ago.