arlyon / async-stripe

Async (and blocking!) Rust bindings for the Stripe API
https://payments.rs
Apache License 2.0
467 stars 131 forks source link

Allow to use owned types on requests #595

Closed lasantosr closed 2 months ago

lasantosr commented 3 months ago

Is your feature request related to a problem? Please describe.

I'm using next branch.

When building requests, for example CreateWebhookEndpoint, we must pass a reference to the fields in order to construct them.

This is inconvenient for two reasons:

Describe the solution you'd like

Update the signature of the methods to require impl Into<Cow<'a, {type}>> and the Builders properties to have Cow<'a, {type}> instead of &'a {type}, allowing both previous scenario and the ones defined above.

For example this method:

/// Construct a new `CreateWebhookEndpoint`.
pub fn new(enabled_events: impl Into<Cow<'a, [CreateWebhookEndpointEnabledEvents]>>, url: impl Into<Cow<'a, str>>) -> Self {
    Self { inner: CreateWebhookEndpointBuilder::new(enabled_events.into(), url.into()) }
}

Would allow both ways:

let url = format!("...");
let enabled_endpoints = vec![...];

// Passing the references, where 'a lives as long as the statements above
let builder1 = CreateWebhookEndpoint::new(&enabled_endpoints, &url);

// Transfering the ownership, where 'a would be the lifetime of the builder itself
let builder2 = CreateWebhookEndpoint::new(enabled_endpoints, url);

Describe alternatives you've considered

There's no alternative other than avoiding those scenarios.

Additional context

No response

mzeitlin11 commented 3 months ago

Thanks for opening this - agreed it would be much more user friendly. I think the related question is if you'd want to keep the borrowed option with Into<Cow<...>> or just take the simpler route and just require Into<OwnedType>.

The latter would make async-stripe codegen much simpler (no lifetimes, no special handling of borrowed types, plus opportunity for codegen to deduplicate these request params with response types). Having to clone unnecessarily sometimes hopefully shouldn't impact performance compared to network requests (some precedence for the simpler route in how other huge generated crates like the aws sdk do it).

The other minor concern to check for would be impact on compile times - one advantage of purely borrowed types is minimal generated code for Drop impls, which show up a surprising amount when using tools like cargo bloat or cargo llvm-lines to investigate compile time.

lasantosr commented 3 months ago

I mentioned Cow just to make use of the already-traked lifetimes and avoid unnecessary clones when not required, but I agree that the completely owned type would be better than just references, at least on usability. Didn't know about the Drop implications tho, but I'd happily spend some more time compiling and less time dealing with the limitations.

I'll try to familiarize with the codegen crate to check if I can make some PR, but I'm not sure if I will get familiar enough to implement that refactor.

arlyon commented 2 months ago

I will close this since it has landed in next and we don't plan on backporting.