Skip to content

XResultHandler should expose MessageProperties creation#378

Open
csmir wants to merge 13 commits into
NetCordDev:mainfrom
csmir:feat/result-messageformatters
Open

XResultHandler should expose MessageProperties creation#378
csmir wants to merge 13 commits into
NetCordDev:mainfrom
csmir:feat/result-messageformatters

Conversation

@csmir

@csmir csmir commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

This PR exposes the GetFailMessageAsync function in:

  • ApplicationCommandResultHandler<T>
  • ComponentInteractionResultHandler<T>
  • CommandResultHandler<T>

In order to allow implementations to change or replace the behavior of failure message creation.

Breaking: The MessageFlags? messageFlags = null parameter in all three result handler implementations has been removed in favor of implementation-first design for pushing the MessageProperties.Flags variable user-side.

An alternative to this approach is considering to expose delegate defaults for these classes, e.g.

public sealed class DelegateApplicationCommandResultHandler(Func<..., ValueTask> resultDelegate) { ... }
configuration.ResultHandler = new DelegateApplicationCommandResultHandler((...) => { ... });

@github-actions

Copy link
Copy Markdown

The documentation preview is available at https://preview.netcord.dev/378.

Comment thread Hosting/NetCord.Hosting.Services/Commands/CommandResultHandler.cs Outdated
@KubaZ2

KubaZ2 commented Jun 25, 2026

Copy link
Copy Markdown
Member

Maybe it would be a good idea to change GetFailMessage to ValueTask<...> GetFailMessageAsync?

csmir and others added 7 commits June 26, 2026 00:33
…onCommandResultHandler.cs

Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
…onCommandResultHandler.cs

Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
…ntInteractionResultHandler.cs

Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
…ntInteractionResultHandler.cs

Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
…ntInteractionResultHandler.cs

Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
…onCommandResultHandler.cs

Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
@csmir

csmir commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Do we care about asynchronicity in such a function? Or are we thinking about potential user-side scenarios that might?

@KubaZ2

KubaZ2 commented Jun 25, 2026

Copy link
Copy Markdown
Member

I can think someone may want to call a database to format a message or something like that.

@csmir

csmir commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

That seems fair, I'll change this then.

@csmir csmir requested a review from KubaZ2 June 25, 2026 22:51
@csmir csmir requested a review from KubaZ2 June 26, 2026 17:03
Comment thread Hosting/NetCord.Hosting.Services/Commands/CommandResultHandler.cs Outdated
Co-authored-by: Kuba_Z2 <77853483+KubaZ2@users.noreply.github.com>
@csmir csmir requested a review from KubaZ2 June 26, 2026 17:13
Comment on lines +49 to +51
var messageProperties = await GetFailMessageAsync(failResult, context, services).ConfigureAwait(false);

return new(interaction.SendResponseAsync(InteractionCallback.Message(message)));
await interaction.SendResponseAsync(InteractionCallback.Message(messageProperties)).ConfigureAwait(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var messageProperties = await GetFailMessageAsync(failResult, context, services).ConfigureAwait(false);
return new(interaction.SendResponseAsync(InteractionCallback.Message(message)));
await interaction.SendResponseAsync(InteractionCallback.Message(messageProperties)).ConfigureAwait(false);
var message = await GetFailMessageAsync(failResult, context, services).ConfigureAwait(false);
await interaction.SendResponseAsync(InteractionCallback.Message(message)).ConfigureAwait(false);

and same for the other result handlers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not compile because message is already reserved on all handlers for upper logic.

@csmir csmir Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* Apologies, on the CommandResultHandler specifically. On others it works. For consistency I believe differentiating is inelegant.

@csmir

csmir commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Please consider my patience for further nitpicky code suggestions. In open source it is most expected that functional and convention-matched code should be ready to merge. If you disagree, write a CONTRIBUTORS document that prepares developers on how to work on NetCord.

I do not mean to insult or look any the wiser. I just do not know what you expect. This back-and-forth on only appearance and no functional changes is time costly.

Please make one final look-through to make this merge-ready with me.

I will not handle any further iterations past the next, you can pick up the pull request yourself if you have anything more to add after this.

@csmir csmir requested a review from KubaZ2 June 26, 2026 18:39
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