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

feat(next): owned types on requests #597

Merged
merged 1 commit into from
Sep 7, 2024
Merged

Conversation

lasantosr
Copy link

@lasantosr lasantosr commented Aug 26, 2024

Summary

This pull request closes #595 by removing borrow info from requests.

I'm not adding Cow because it would add unnecessary complexity, but I did add the impl Into<{type}> as it's convenient and it doesn't incur in performance penalty when both types are the same.

I didn't remove the lifetime code from the repo because I'm not sure if it might be useful somewhere else, I've just set .can_borrow(false) on the top-level inferences.

Checklist

@lasantosr
Copy link
Author

@mzeitlin11 @arlyon do you think you can have a look at this PR for the next branch?

@mzeitlin11
Copy link
Collaborator

@mzeitlin11 @arlyon do you think you can have a look at this PR for the next branch?

Thanks for the PR - I hope to be able to look this weekend!

@mzeitlin11
Copy link
Collaborator

Thanks, this is cool! If you don't mind, I'd like to cherrypick this comment and investigate building off this to make more invasive changes. I think removing the notion of borrowing like done here will allow unifying the handling between "request params" and general "stripe types", hopefully allowing some more simplifications.

@lasantosr
Copy link
Author

Thanks for having the time to review it!

I think this can be a first step, so that we can get rid of lifetimes in the generated code and start testing those changes.

Then, another PR can be made to the codegen crate in order to allow for such improvements and simplifications, which shouldn't affect, at least not much, the generated code.

@mzeitlin11
Copy link
Collaborator

That sounds good to me too! Happy to merge as is, but will give some time for @arlyon to look if he wants before merging. (In the meantime, anybody wanting to test this out can always depend directly on this branch)

@arlyon
Copy link
Owner

arlyon commented Sep 3, 2024

I am for this. ultimately stripe code will not be on some performance critical path so any extra clones are probably fine. Having to enforce container types is a bit of a shame but I think this is a shortcoming of rust in general rather than this crate. Either you have unowned types and deal with the awkward lifetimes or you have owned types and need to convert stuff. I do not want to end up with some situation where we are generic over the container as I feel compile times would suffer for what is a fairly minor performance win.

Although, for the sake of exhaustiveness, in the examples linked I would in the first case implement from from a reference to MyType (assuming, again that lifetimes work) and in the second case something like this could work:

let mut builder = CreateWebhookEndpoint::new(&enabled_events, &url);
let description = if description_updated {
    Some(format!("Endpoint for {url}"));
} else {
    None
};
let builder = if let Some(d) = description.as_ref() {
    builder.set_description(d);
} else {
    builder
};
builder = builder.api_version(VERSION);

@arlyon
Copy link
Owner

arlyon commented Sep 3, 2024

Looks like the tests issue is stripe mock moving out from underneath us. #602 fixes this so you should be able to rebase

Also, thanks for taking the time to make this contribution! 🎉 Will come back for a final review once those tests are passing. If you want to run them locally you will need to run the stripe mock container. Have a look at this section in CI for the commands:

https://github.com/arlyon/async-stripe/actions/runs/10670068473/workflow?pr=597#L128-L155

@lasantosr
Copy link
Author

@arlyon It should be ready to merge

@arlyon arlyon merged commit df8a348 into arlyon:next Sep 7, 2024
15 checks passed
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

Successfully merging this pull request may close these issues.

3 participants