edgedb-rust icon indicating copy to clipboard operation
edgedb-rust copied to clipboard

Restructure edgedb-protocol

Open CodesInChaos opened this issue 4 years ago • 8 comments

I'd expose 4 public modules from edgedb-protocol:

  1. model - Types an application will need in its model
    • BigInt, Decimal
    • Duration, LocalDatetime, LocalDate, LocalTime
    • Re-export of uuid::Uuid
    • TBD: what to do about Datetime vs SystemTime and json
  2. dynamic/value - Value, NamedTuple and related types which are needed to represent unknown edgedb data at runtime
  3. serialization - traits for serialization, de-serialization (currently Queryable) and related types
  4. messages - probably should split this one into a separate crate (or rather move the rest to a separate crate like edgedb-data)

CodesInChaos avatar Aug 16 '20 11:08 CodesInChaos

I'm fine with all names except "model". I suggest "datatypes" or "data_types" or even "dt". Is the module going to be used frequently, btw?

1st1 avatar Aug 18 '20 18:08 1st1

model - Types an application will need in its model

IMO, data_types or scalars (if we only put implementations of scalar types there) is a better name (see also the discussion in #44).

Queryable

Why Queryable and not Deserialize?

elprans avatar Aug 18 '20 18:08 elprans

Is the module going to be used frequently, btw?

Originally I though, yes. But reconsidering it, I'm not so certain. The most popular types map to standard rust types, especially since points in time (edgedb's Datetime) can be mapped to SystemTime. The popularity of the other date/time types depend a lot of the type of application and even there the application might choose to use types from another crate, like chrono if edgedb implements Queryable for them. Uuid is the big unknown for me, but I'd guess identifying objects would be pretty common.

Why Queryable and not Deserialize?

That's what it's called at the moment. I also prefer names like Decode or Deserialize.

Deserialize has the minor downside that it matches a trait defined by the popular Serde crate, which might cause some confusion.


I'll update this pull request to call it data_types then.

CodesInChaos avatar Aug 18 '20 20:08 CodesInChaos

I also prefer names like Decode

Yes, we use the "decode", and "encode" terminology in other bindings and a piece of code that combines both is called a "codec".

elprans avatar Aug 18 '20 20:08 elprans

I'd expose 4 public modules from edgedb-protocol:

1. `model` - Types an application will need in its model
   
   * `BigInt`, `Decimal`
   * `Duration`, `LocalDatetime`, `LocalDate`, `LocalTime`
   * Re-export of `uuid::Uuid`
   * TBD: what to do about `Datetime` vs `SystemTime` and json

Sounds good. I don't think we need to re-export SystemTime if you ask that. #42 is fine.

2. `dynamic`/`value` - `Value`, `NamedTuple` and related types which are needed to represent unknown edgedb data at runtime

dynamic::Value looks good. By NamedTuple you mean the one that part of Value, Not the one that has QueryArg implemented, right?

3. `serialization` - traits for serialization, de-serialization (currently `Queryable`) and related types

This is a good question. Does it contain all the implementations of the Queryable for all the types? So it would likely contain submodules?

4. `messages` - probably should split this one into a separate crate (or rather move the rest to a separate crate like `edgedb-data`)

I've thought a bit about latter. I.e. making edgedb-types crate. But I'm not sure if orphan rules would allow edgedb-types do not depend on edgedb-protocol. Because otherwise this is not worth it.

Why Queryable and not Deserialize?

That's what it's called at the moment. I also prefer names like Decode or Deserialize.

Deserialize has the minor downside that it matches a trait defined by the popular Serde crate, which might cause some confusion.

  1. Queryable name was stolen from the diesel so we have precedent of using the name in rust.

  2. We have precedent of using both serde::Deserialize and Queryable in the same file:

    #[derive(Queryable)]
    struct MyResponse {
       #[edgedb(json)]
        json_field: Structure2,
    }
    #[derive(Deserialize)]
    struct Structure2 { ... }
    

    Using namespaces there will make users' lifes harder

  3. Decode vs Deserialize are too similar names that I would likely mess up with them every now and than. It's hard to remember that Decode is for edgedb and Deserialize is for say JSON. Queryable is much more memorable to describe the type that can be queried from the database.

It's not that Queryable is a perfect name. It can be QueryResult like suggested in #32, or maybe some other name, but I'm reluctant to accept Decode or Deserialize.

tailhook avatar Aug 19 '20 11:08 tailhook

I'd expose 4 public modules from edgedb-protocol:

  1. model - Types an application will need in its model

    • BigInt, Decimal
    • Duration, LocalDatetime, LocalDate, LocalTime
    • Re-export of uuid::Uuid
    • TBD: what to do about Datetime vs SystemTime and json
  2. dynamic/value - Value, NamedTuple and related types which are needed to represent unknown edgedb data at runtime

  3. serialization - traits for serialization, de-serialization (currently Queryable) and related types

  4. messages - probably should split this one into a separate crate (or rather move the rest to a separate crate like edgedb-data)

Someone recently introduced me to Google's FlatBuffer. Unless the Datetime vs SystemTime and json problem is solved, FlatBufferBuilders make a strong case for this (and other) tricky models to deterministically type in the edgedb-protocol at runtime.

dmgolembiowski avatar Oct 08 '20 23:10 dmgolembiowski

Someone recently introduced me to Google's FlatBuffer. Unless the Datetime vs SystemTime and json problem is solved, FlatBufferBuilders make a strong case for this (and other) tricky models to deterministically type in the edgedb-protocol at runtime.

The title might be misleading, but it's about restructuring Rust crate not the protocol itself. For now we don't plan any changes on serialization of things.

tailhook avatar Oct 09 '20 13:10 tailhook

I would also like to shorten crate names, typing edgedb_protocol seems unnecessarily verbose

MrFoxPro avatar Mar 27 '24 13:03 MrFoxPro