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

fix(progenitor-macro): if 'client.inner' is not set by user do not provide it to user-specified functions. #933

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JosiahBull
Copy link

@JosiahBull JosiahBull commented Oct 1, 2024

Description

When adding any hooks (pre_async, post, pre) you were implicitly required to add an empty type just to fill in client.inner property - even if you weren't actually using the client for anything. This mean that if you set an async hook without setting an inner client progenitor would create invalid code.

We have changed that behaviour to only generate the inner parameter if it's actually set by the end user.

Previous Code

// an inner type MUST be set to use with_pre_hook_async?!
// if we exclude this the generator succeeds but the generated code doesn't compile!
.with_inner_type(quote! { () })

// The actual hook...
.with_pre_hook_async(quote! {
    // We aren't even using the client.inner property!
    |_, request: &mut reqwest::Request| {
        Box::pin(async { Ok::<_, Box<dyn std::error::Error>>(()) })
    }
})

After Change

.with_pre_hook_async(quote! {
    |request: &mut reqwest::Request| {
        Box::pin(async { Ok::<_, Box<dyn std::error::Error>>(()) })
    }
})

Future Work

Given we have a token stream of the hook body, we could try to identify the parameters required and be a little smarter... but this is already a big improvement.

@upachler
Copy link

upachler commented Oct 8, 2024

Thanks for providing this, I came across this problem myself last week (and I 'fixed' it by providing documentation in this PR #934, which I'm happy to adapt once yours gets accepted).

One thing to think about in future may be to move the inner type out of the scope of the code generator entirely, as well as the hook functions. So instead of generating a Client, which gets the inner type T and the hook functions /baked in/ via the generator, I'd rather like something like a Client<T> being generated where T is the inner type which may or may not implement certain traits like PreHook or PostHook.

This way, you can generate to client in a separte crate, but provide inner type and hook functions in the crate where you use them. This would allow to ship crates with a generated client that can then be augmented with inner type and hook function wherever it is used, without the need for a generator. Also I find hook development easier this way, because all the macro magic no longer gets in the way.

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.

2 participants