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

Discussion: Future of command contexts and making contextual information more verbose #153

Open
VelvetToroyashi opened this issue Jan 31, 2022 · 3 comments

Comments

@VelvetToroyashi
Copy link
Contributor

Myself personally, and I'd assume some others have stumbled upon the issue of not being able to detremine what really happened upon command execution.

@Nihlus pointed out three distinct solutions that all have various pros in cons and regards to how to solve this issue.

Solution 1: Expanding ICommandContext

This one is a simple change, and would add an IChildNode or the likes to the pre-existing ICommandContext and it's derivitives.

This would more than likely be an Optional<T> property, seeing as contexts (namely InteractionContext) have had their usage scope expanded to more than just traditional commands (re: Interactivity)

Solution 2: Node injection service

This change is also relatively simple, and piggybacks off a similar system that's currently in use by the command responders.

A new service would be added, by the names of NodeInjectionService or the likes. It will posses the current command node being invoked, much like ContextInjectionService.

Solution 3: Reworking context architecture

This solution is largest of the three, and involves reworking how contexts are handled and constructed.

Concrete types would posess a node property only if applicable, being omitted from the others.

ICommandContext would be minimally affected by this, as new base classes would be rewritten to derive it, especially in the case of interactions, where an IInteractionContext class would be spawned, and InteractionContext and InteractionCommandContext would derive from the former.

@gqvz
Copy link

gqvz commented Jun 23, 2022

I've also stumbled upon this issue, there have been several instances where I need to check what command was the reason for a event or condition, but its seemingly impossible or a huge pain to implement without library level support, and there's also this

do solution 3, the context types are pretty lame as-is
- Maxine

@Hamsterland
Copy link
Contributor

Hamsterland commented Jun 23, 2022

I've also stumbled upon this issue, there have been several instances where I need to check what command was the reason for a event or condition, but its seemingly impossible or a huge pain to implement without library level support, and there's also this

do solution 3, the context types are pretty lame as-is
- Maxine

You can return whatever Result you want from your command.

For example, I want to log every moderation action (inside of an IPostExecutionEvent) taken after a moderation command is invoked. How does my post execution event know it was a moderation command that returned and not something else? I simply just return moderation-specific data within the result and do a check in the event.

return Result<InfractionPostExectionData>.FromSuccess(new InfractionPostExectionData(createInfractionResult.Entity));
public async Task<Result> AfterExecutionAsync
(
    ICommandContext context,
    IResult commandResult,
    CancellationToken ct = default
)
{
    if (!commandResult.IsSuccess)
    {
        return Result.FromSuccess();
    }
    
    if (commandResult is not Result<InfractionPostExectionData> postExecutionDataResult)
    {
        return Result.FromSuccess();
    }
}

You could then extend this to simply return the type of the containing class and MethodInfo of the command invoked.

I know this is not really a solution to what you're saying, but for now, it is a good workaround.

@gqvz
Copy link

gqvz commented Jun 23, 2022

@Hamsterland thanks for the workaround, i will be using that for now, but that still leaves preexecutionevents and conditions, i hope one of the solutions velvet listed gets added soon:tm:

Nihlus added a commit that referenced this issue Nov 22, 2022
[WIP] Rework context types (#153, solution 3)
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 a pull request may close this issue.

3 participants