Lonami / grammers

(tele)gramme.rs - use Telegram's API from Rust
https://t.me/gramme_rs
Apache License 2.0
561 stars 116 forks source link

Add serde support for tl_types. #277

Closed RuofengX closed 1 month ago

RuofengX commented 1 month ago

Add two lines for each entrance in generated.rs

Pass all tests in crate grammers-tl-types

RuofengX commented 1 month ago

Related issue #276

RuofengX commented 1 month ago

Whoops, generated code seems encount some issue.

Lonami commented 1 month ago

Can you revert the formatting change to the .toml files out of this PR?

RuofengX commented 1 month ago

I'm not sure I understand why so many crates need the serde dependency. Shouldn't it only be grammers-tl-types?

I tried to add serde code only in grammers-tl-types first, all tests pass in tl-types crates.

Then I add my fork of grammers-client in my project, it compile with error saying that the generated generated.rs(that long file) missing dependence serde. I checked it twice, the error is not from grammers-tl-types, it from grammers-seesion. Then I added serde and serde-derive dep in all crates that depend on grammers-tl-types. Thus no error again.

I not formillar with build system, but I am looking into it.


Also, please make it optional. I want to continue offering the crates with serde off by default.

Sure, I will write cfg(feature="serde") in grammers-tl-gen crate, then added a new feature in grammers-client after first issue done.

RuofengX commented 1 month ago

I think it's done. Our work is never over.

Behavior now

User can fetch any raw struct (defined by grammers_tl_types) returned by client function after enable the serde feature tag.

Flaw

Structure in grammers_client like Client, Update still not deserializable, because it contains runtime data. Further work need to be done. I will implement only serialize for those structure soon.

Example

https://github.com/RuofengX/gray-mirror-tg/blob/3a6b343184307b9ccb9c66a74fb64bc8c041577c/src/app/mod.rs#L110

RuofengX commented 1 month ago

We need more test and docs about this new feature.

Lonami commented 1 month ago

First PRs require me to click the Approve button every time to have CI run, so it might take a while until I realize I had to click it again.

RuofengX commented 1 month ago

We have some Vec<Vec<u8>> type now in generated.rs, and it cannot get parsed by serde_bytes, like CodeSetting.logout_tokens, it is a Vec<Vec<u8>>.
First vector is a list, while second is a bytes.

I don't know whether Vec<#[serde(with = "serde_bytes")] Vec<u8>> will work. Moreover current inject point cannot capture the exact begin of a bytes type, it can only add mark before definition line. The nested write processing wipes the boundry between list and bytes.

Plan 1. If Vec<#[serde(with = "serde_bytes")] Vec<u8>> works, we could write serde_bytes mark just before bytes type parsed. I can foresee a lot refactor to pass params.

Plan 2. Maybe we could add a type alia in generated file to distinguish bytes and vector in generate logic.

Plan 3. We could use 3-rd party dependence like bytes from tokio team to support serde and nested type more easily. But this will bring more dependences and limit downstream user's choice.

Plan 4. Leave this as a flaw, do nothing about it.

Anyway, I fix the broken test, it is because the blank space in generated code. And revert to the commit that no new duplicate test in grammers-tl-types.

Lonami commented 1 month ago

Plan 4. Leave this as a flaw, do nothing about it.

I think this is fine. Needing to serialize types like that seems extremely unlikely. As long as it works, even if it's inefficient, I think it's fine.