edgedb / edgedb-rust

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

Restructure edgedb-protocol #38

Open CodesInChaos opened 3 years ago

CodesInChaos commented 3 years ago

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)
1st1 commented 3 years ago

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?

elprans commented 3 years ago

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?

CodesInChaos commented 3 years ago

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.

elprans commented 3 years ago

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".

tailhook commented 3 years ago

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.

dmgolembiowski commented 3 years ago

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.

tailhook commented 3 years ago

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.

MrFoxPro commented 3 months ago

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