Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to use owned types on requests #595

Closed
lasantosr opened this issue Aug 23, 2024 · 3 comments
Closed

Allow to use owned types on requests #595

lasantosr opened this issue Aug 23, 2024 · 3 comments

Comments

@lasantosr
Copy link

lasantosr commented Aug 23, 2024

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:

  • We can't implement From other types we own to the requests or build the requests in any isolated method, because we can't return values referenced by the function and we can't transfer the ownership. Pseudo-code example:

    impl<'a> From<MyType> for CreateWebhookEndpoint<'a> {
        fn from(value: MyType) -> Self {
            // Not working 
            Self::new(&value.enabled_events, &value.url)
        }
    }
  • We can't build the values only when required, because borrowed value does not live long enough. Pseudo-code example:

    let mut builder = CreateWebhookEndpoint::new(&enabled_events, &url);
    if description_updated {
        let new_description = format!("Endpoint for {url}");
        // Not working 
        builder = builder.description(&new_description);
    }
    builder = builder.api_version(VERSION);

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Owner

arlyon commented Sep 24, 2024

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

@arlyon arlyon closed this as completed Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants