google / tarpc

An RPC framework for Rust with a focus on ease of use.
MIT License
3.18k stars 194 forks source link

Add a way to derive Clone for the generated Request and Response types #436

Closed ShaneMurphy2 closed 6 months ago

ShaneMurphy2 commented 7 months ago

Currently tarpc::service creates request and response types that are not clone. This makes it difficult it difficult to test and mock. It would be nice if the service macro used an attribute macro to control adding derives for things like PartialEq, Clone, or Hash. Is there any reason not to do this? If there isn't, I can take a stab at it.

tikue commented 7 months ago

Thanks for the suggestion! I agree this could be supported.

It'd be nice to replace the derive_serde arg with something generic enough to support this new use case. Maybe something like this would be possible:

#[tarpc::service(derive = [tarpc::serde::Serialize, tarpc::serde::Deserialize, Clone, Hash])]
service World {
    async fn hello(message: String) -> String;
}
ShaneMurphy2 commented 7 months ago

@tikue Got a first cut up on a branch: https://github.com/ShaneMurphy2/tarpc/tree/customizable-derives, let me know what you think so far.

Still need to see what breaks by changing the macro syntax, but the core use case works.

tikue commented 7 months ago

Looks nice! It'd be nice for derive_serde and derive to be mutually exclusive, so that people are encouraged to make the switch. In particular:

ShaneMurphy2 commented 7 months ago

Looks nice! It'd be nice for derive_serde and derive to be mutually exclusive, so that people are encouraged to make the switch. In particular:

  • derive_serde and derive at the same time should be an error
  • derive should only derive Serialize/Deserialize when they are explicitly listed
  • derive_serde is deprecated - output a warning when it is used, and suggest instead using either derive = [] to disable, or derive = [Serialize, Deserialize] to explicitly enable
  • for backwards compatibility, absence of derive can derive Serialize/Deserialize by default.

On it 👍

ShaneMurphy2 commented 7 months ago

@tikue PR is up!