HTTP Interactions #1999
HTTP Interactions #1999
Conversation
|
Should be ready to merge, not sure about the packaging of the new project and maybe its possible to reduce allocs |
DSharpPlus.HttpInteraction.AspNetCore/IEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
akiraveliara
left a comment
akiraveliara
left a comment
There was a problem hiding this comment.
in addition to my comments, is there a reason the extension is called DSharpPlus.HttpInteraction... in the singular, but the handling code in DSharpPlus.Net.HttpInteractions is in the plural
DSharpPlus.HttpInteraction.AspNetCore/DSharpPlus.HttpInteraction.AspNetCore.csproj
Outdated
Show resolved
Hide resolved
DSharpPlus.HttpInteraction.AspNetCore/IEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
DSharpPlus.HttpInteraction.AspNetCore/IEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
DSharpPlus.HttpInteraction.AspNetCore/DSharpPlus.HttpInteraction.AspNetCore.csproj
Outdated
Show resolved
Hide resolved
DSharpPlus.HttpInteraction.AspNetCore/IEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
VelvetToroyashi
left a comment
VelvetToroyashi
left a comment
There was a problem hiding this comment.
On the surface, looks fine. I do have some other concerns, which I voiced in the libdev channel, but I've commented on immediate issues above.
DSharpPlus.HttpInteractions.AspNetCore/EndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # DSharpPlus/Clients/BaseDiscordClient.cs # DSharpPlus/Net/Abstractions/Transport/TransportApplication.cs
| { | ||
| if (this.taskCompletionSource.Task.IsCanceled) | ||
| { | ||
| throw new InvalidOperationException( |
There was a problem hiding this comment.
This isn't necessarily indicative of Discord closing the connection though?
There was a problem hiding this comment.
The cancelation token should be tied to the http connection. Maybe i should add additional docs, in aspnet you get a ct which is cancled when the request is cancled
There was a problem hiding this comment.
but in general it can be that the dev cancels the interaction with other intentions
There was a problem hiding this comment.
I think velvet means that the http connection could also be closed due to other reasons, such as a random network drop?
There was a problem hiding this comment.
This is likely...
I think its the common case.
DSharpPlus.HttpInteractions.AspNetCore/EndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
Naamloos
left a comment
Naamloos
left a comment
There was a problem hiding this comment.
Generally looks good to me. Seems safe to merge when it has been tested
* basic structure * Improve Bindings, implement VerifySignature and implement DiscordHttpInteraction * Add http interaction injection * Respond to pings * Correctly parse the interaction * remove nested sln * Make interaction cancelble to cancel on http request cancelation * Add AspNetCore extension and add verifykey to DiscordApplication * Add missing xml docs * fix formatting, project name and csproj config * Add PackageId * Remove RootNamespace from csproj * Maybe write the bytes directly, untested * Apply namespace rename and remove unused usings * Package description and tags and also reword method description * explicitly add outputtype and add another package tag * Better exception if discord closed the connection * fix merge mistake * remove NL * Add CancellationToken in HandleDiscordInteractionAsync in aspnet package * another missing ct * and another ct * Take ArraySegments to allow more optimizations * XMLDocs and formatting * add mising xmldoc * Parse the interaction like we do it in D*spatch
* basic structure * Improve Bindings, implement VerifySignature and implement DiscordHttpInteraction * Add http interaction injection * Respond to pings * Correctly parse the interaction * remove nested sln * Make interaction cancelble to cancel on http request cancelation * Add AspNetCore extension and add verifykey to DiscordApplication * Add missing xml docs * fix formatting, project name and csproj config * Add PackageId * Remove RootNamespace from csproj * Maybe write the bytes directly, untested * Apply namespace rename and remove unused usings * Package description and tags and also reword method description * explicitly add outputtype and add another package tag * Better exception if discord closed the connection * fix merge mistake * remove NL * Add CancellationToken in HandleDiscordInteractionAsync in aspnet package * another missing ct * and another ct * Take ArraySegments to allow more optimizations * XMLDocs and formatting * add mising xmldoc * Parse the interaction like we do it in D*spatch
4 participants
Summary
This pr aim to provide users with a way to inject interactions into the client which are invoked through webhooks
Notes
Everything tested, missing docs article