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

#673 allow to access FlurlCall in RespondWith #825

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

Conversation

RytisLT
Copy link

@RytisLT RytisLT commented May 15, 2024

Minimal change that adds FlurlCall to RespondWith allowing to create response based on request parameters like body, query parameters, etc.

@tmenier
Copy link
Owner

tmenier commented May 16, 2024

Thanks. I'll look at this more closely when I have some time, but I like the general idea. I'm wondering if a FlurlRequest arg instead of FlurlCall might be enough? It seems intuitive: "given this request, return this response". Also, I could see people wanting to do this with RespondWith(string) and RespondWithJson(object) too, since those are probably more commonly used than the low-level method. So we might want overloads for those.

@RytisLT RytisLT force-pushed the dev branch 3 times, most recently from 2f9507e to 97d3858 Compare May 16, 2024 07:17
@RytisLT
Copy link
Author

RytisLT commented May 16, 2024

@tmenier I've changed Func<FlurlCall, HttpContent> to Func<IFlurlRequest, HttpContent> and added the two overloads for string and Json. There is one caveat, to get to request body I need to call request.Content.ReadAsStringAsync().Result since I cannot await in the non-async method. To make it proper async we would need to introduce RespondWithAsync, this would require more changes to the code. I could take a stab at it if you want.

@RytisLT
Copy link
Author

RytisLT commented May 21, 2024

@tmenier would you like to see any more changes to the PR or does it look good the way it is?

@RytisLT
Copy link
Author

RytisLT commented May 23, 2024

@tmenier can you please have a look at the PR I really need this feature for my project. Thanks!

@RytisLT
Copy link
Author

RytisLT commented Jun 6, 2024

@tmenier any chances on getting this merged?

@tmenier
Copy link
Owner

tmenier commented Jun 7, 2024

New features are more than just a merge. I haven't had time to work on the next sprint so you'll have to be patient or use your own fork.

@RytisLT
Copy link
Author

RytisLT commented Jun 7, 2024

Roger that. Can I help you out in any 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