feature: builder pattern for application
WhyNotHugo opened this issue · 3 comments
Creating an Application instance is pretty noisy:
let immediate_alert_service: Uuid = Uuid::from_u16(IMMEDIATE_ALERT_SERVICE);
let alert_level: Uuid = Uuid::from_u16(ALERT_LEVEL);
let app = Application {
services: vec![Service {
uuid: immediate_alert_service,
primary: true,
characteristics: vec![Characteristic {
uuid: alert_level,
write: Some(CharacteristicWrite {
write_without_response: true,
method: CharacteristicWriteMethod::Fun(Box::new(move |new_value, _req| {
async move {
println!("Got an alert with value: {:?}", new_value);
Ok(())
}
.boxed()
})),
..Default::default()
}),
..Default::default()
}],
..Default::default()
}],
..Default::default()
};All these Default::default add lot of noise to reading the code (I'm pretty sure the compiler actually optimised away all the intermediate variables being created here).
After writing a small application using this type, I have a strong impression that a much better API could be provided by using a builder pattern:
let immediate_alert_service: Uuid = Uuid::from_u16(IMMEDIATE_ALERT_SERVICE);
let alert_level: Uuid = Uuid::from_u16(ALERT_LEVEL);
let app = Application::buider()
.add_service(
Service::builder(immediate_alert_service).with_primary_characteristic(
Characteristic::builder(alert_level).write_without_response(
true,
CharacteristicWriteMethod::Fun(Box::new(move |new_value, req| {
async move {
todo!("{:?} {:?}", new_value, req);
}
.boxed()
})),
),
),
)
.build();Note how this approach also makes some fields mandatory (e.g.: the uuid to Application must be provided). Aside from with_primary_characteristic, the ServiceBuilder also has a with_characteristic for non-primary ones.
A longer (fictitious) example:
let app = Application::builder()
.add_service(
Service::builder(immediate_alert_service)
.with_primary_characteristic(
Characteristic::builder(alert_level).write_without_response(
true,
CharacteristicWriteMethod::Fun(Box::new(move |new_value, req| {
async move {
todo!("{:?} {:?}", new_value, req);
}
.boxed()
})),
),
)
.with_characteristic(
Characteristic::builder(another_uuid).write_without_response(
true,
CharacteristicWriteMethod::Fun(Box::new(move |new_value, req| {
async move {
todo!("{:?} {:?}", new_value, req);
}
.boxed()
})),
),
),
)
.add_service(
Service::builder(another_service).with_primary_characteristic(
Characteristic::builder(some_other_uuid).write_without_response(
true,
CharacteristicWriteMethod::Fun(Box::new(move |new_value, req| {
async move {
todo!("{:?} {:?}", new_value, req);
}
.boxed()
})),
),
),
)
.build();Is there any interest in having such builders? I'd start out with the inner ones (e.g.: Characteristic::builder) and progressively add the higher one.
This would be a good extension to the API.
It would also make sense to provide a wrapper function around CharacteristicWriteMethod::Fun and similar types that perform the boxing of the specified closure, i.e. Box::new(move |...| { async move { ... }.boxed() }).
I'd start out with the inner ones (e.g.: Characteristic::builder) and progressively add the higher one.
Yes, we can then discuss the proposed builder pattern in more detail.
It would also make sense to provide a wrapper function around CharacteristicWriteMethod::Fun and similar types that perform the boxing of the specified closure, i.e. Box::new(move |...| { async move { ... }.boxed() }).
Yeah, that's also kind ugly and repetitive. Ideally, the following:
CharacteristicWriteMethod::Fun(Box::new(move |new_value, req| {
async move {
todo!("{:?} {:?}", new_value, req);
}
.boxed()
})),Could be written as:
CharacteristicWriteMethod::from(async move {
todo!("{:?} {:?}", new_value, req);
}),However, async closures are unstable: rust-lang/rust#62290.
I used bluer/examples/gatt_server_cb.rs for some experimenting around simplifying this bit anyway. I ended up hitting lifetime issues that come up due to the async block moving variables into its scope.
I'm not sure that there's anything actionable on simplifying CharacteristicWriteMethod definition.
You don't need async closures for that. You can create a wrapper function like this:
use futures::future::FutureExt;
use std::future::Future;
use std::io::Result;
use std::pin::Pin; // 0.3.30
pub type CharacteristicWriteFun =
Box<dyn Fn(Vec<u8>, String) -> Pin<Box<dyn Future<Output = Result<()>> + Send>> + Send + Sync>;
pub enum CharacteristicWriteMethod {
Fun(CharacteristicWriteFun),
Io,
}
impl CharacteristicWriteMethod {
pub fn fun<Fut>(f: impl Fn(Vec<u8>, String) -> Fut + Send + Sync + 'static) -> Self
where
Fut: Future<Output = Result<()>> + Send + 'static,
{
Self::Fun(Box::new(move |a, b| f(a, b).boxed()))
}
}
fn main() {
CharacteristicWriteMethod::fun(|a, b| async move { Ok(()) });
}