Extension methods to convert VoiceResponse and MessagingResponse to TwiMLResult #86
Conversation
|
I have an idea for merging the two |
|
This is what I was fearing: app.MapGet("/sms",()
=> new MessagingResponse()
.Message($"Ahoy!")
.ToTwiMLResult()
);This code results in the following error: This is because namespace Even when explicitly telling minimal APIs the return type is |
|
What do you think about merging those two classes? In that case there will be only one extension method. |
|
One way to circumvent this, which maybe something we should do anyway, is to merge the This would be the MVC version implementing namespace Twilio.AspNet.Core
{
// ReSharper disable once InconsistentNaming
public partial class TwiMLResult : IActionResult
{
...
public Task ExecuteResultAsync(ActionContext actionContext)
=> WriteTwimlToResponse(actionContext.HttpContext);
private Task WriteTwimlToResponse(HttpContext httpContext)
{
if (Data == null)
{
Data = "<Response></Response>";
}
httpContext.Response.ContentType = "application/xml";
httpContext.Response.ContentLength = Encoding.UTF8.GetByteCount(Data);
return httpContext.Response.WriteAsync(Data);
}
}
}And this would be the Minimal API version implementing namespace Twilio.AspNet.Core;
public partial class TwiMLResult : IResult
{
/// <summary>
/// Writes the TwiML to the HTTP response body
/// </summary>
/// <param name="httpContext">The HttpContext containing the Response to write the TwiML to</param>
public Task ExecuteAsync(HttpContext httpContext) => WriteTwimlToResponse(httpContext);
}The latter would be conditionally compiled only for .NET 6 and up. |
|
@Giorgi Yes, this is where my head is going too. I'm a little annoyed that we didn't do it in the first place. |
|
Agreed, but it's better late than never. If you decide to go that way I can send a PR. |
|
I think it's the way to go. @dprothero What do you think? |
|
I'm not following why this would be a breaking change? |
|
It depends on what folks are doing in their code. I guess we could keep a class at There would also be some signature changes of methods to use the All in all, minor and unlikely breaking changes, but definitely possible. |
|
Also, the namespace |
It won't if the extension method will be available only on the main |
|
@Giorgi I meant an ambiguous reference to the |
|
@dprothero I added a branch @Giorgi I think the best course of action is to do the right thing even if it's breaking change. So I'd merge the |
|
@Giorgi would you like me to move this forward? |
|
Yes, I'll find time to update it. |
|
@Giorgi I'd be happy to move this forward if you can't find the time. All good! |
|
@Swimburger I'll try to do it this week. |
Implements #61
As there are two
TwiMLResultclasses (one for Minimal API and one for controller-based approach) I had to create to classes for the extension methods and put them in the respective namespaces.Contributing to Twilio