cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.25k stars 1.1k forks source link

ServerConf::from_yaml() & Co. should be implemented via a trait #232

Open palant opened 1 month ago

palant commented 1 month ago

What is the problem your feature solves, or the need it fulfills?

Extending ServerConf with custom settings works along these lines:

#[derive(Debug, PartialEq, Eq, Deserialize)]
#[serde(default)]
pub struct MyAppConf {
    custom_setting: bool,

    #[serde(flatten)]
    server: ServerConf,
}

The problem is, ServerConf::from_yaml() and similar methods are not defined for this structure and have to be reimplemented.

Describe the solution you'd like

The methods from_yaml() and load_from_yaml() are generic and should be defined on a trait like FromYaml. These can then get a blanket implementation for anything implementing serde::Deserialize. This way any custom extension of ServerConf will have that functionality implemented automatically.

Method to_yaml() can also be a blanket implementation for anything implementing serde::Serialize.

This will make extending the default configuration easier. Another positive side-effect: serde_yaml will stay a dependency of Pingora and won’t have to be pulled in for the applications using it.

Describe alternatives you've considered

It isn’t too hard to reimplement this functionality for custom extensions of course, just unnecessary.

palant commented 1 month ago

For reference, so far I’m using my own implementation of this trait which can be found here: https://github.com/palant/pingora-utils/blob/main/module-utils/src/lib.rs#L89-L124.