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

Restructure edgedb-protocol #38

Open
CodesInChaos opened this issue Aug 16, 2020 · 8 comments
Open

Restructure edgedb-protocol #38

CodesInChaos opened this issue Aug 16, 2020 · 8 comments

Comments

@CodesInChaos
Copy link
Contributor

CodesInChaos commented Aug 16, 2020

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)
tailhook added a commit to geldata/gel-cli that referenced this issue Aug 17, 2020
The crash is when a last brace `}` doesn't fit screen width.

This only happened on the stream of objects printed so was not triggered
the tests before. This commit refactors tests to trigger that crash and
fixes the bug.

Fixes #125, supersedes geldata/gel-rust#38
tailhook added a commit to geldata/gel-cli that referenced this issue Aug 17, 2020
The crash is when a last brace `}` doesn't fit screen width.

This only happened on the stream of objects printed so was not triggered
the tests before. This commit refactors tests to trigger that crash and
fixes the bug.

Fixes #125, supersedes geldata/gel-rust#38
@1st1
Copy link
Member

1st1 commented Aug 18, 2020

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
Copy link
Member

elprans commented Aug 18, 2020

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

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
Copy link
Member

elprans commented Aug 18, 2020

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

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

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

tailhook commented Oct 9, 2020

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

MrFoxPro commented Mar 27, 2024

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

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

No branches or pull requests

6 participants