KnightHacks / scythe

A Command-Driven Discord.js Addon
MIT License
3 stars 3 forks source link

Add message filters. #17

Closed suneettipirneni closed 3 years ago

suneettipirneni commented 3 years ago

Message filters are middleware that gets run whenever a message is sent to decide whether it should be deleted or not. A common example of this is deleting messages in a counting channel where a number is not sequential.

rob-3 commented 3 years ago

Good feature overall. I think this fits better inside of the new registerInteractionListener.ts module in #18 which is responsible for managing incoming interactions and the state associated with registerUI. Under the design we're trying, I would pass in registerMessageFilters as a parameter to run() to remove the coupling to Client and keep SRP happy.

Also, I feel like you don't need to dynamically import message filters. They make the most sense to me as free functions in the file with the command that wants to implement them. This also will let us typecheck them at compile time before the signatures get erased.

suneettipirneni commented 3 years ago

Good feature overall. I think this fits better inside of the new registerInteractionListener.ts module in #18 which is responsible for managing incoming interactions and the state associated with registerUI. Under the design we're trying, I would pass in registerMessageFilters as a parameter to run() to remove the coupling to Client and keep SRP happy.

That's fine

Also, I feel like you don't need to dynamically import message filters. They make the most sense to me as free functions in the file with the command that wants to implement them. This also will let us typecheck them at compile time before the signatures get erased.

I'd prefer to offer both options

rob-3 commented 3 years ago

I'd prefer to offer both options

What is the use case for dynamically importing? I feel like we just lose the type safety for nothing. The reason I feel the dynamically imported Commands are useful is so we don't have to keep an array of every command updated somewhere, but I feel like that would be a nonissue for the filters.

suneettipirneni commented 3 years ago

I'd prefer to offer both options

What is the use case for dynamically importing? I feel like we just lose the type safety for nothing. The reason I feel the dynamically imported Commands are useful is so we don't have to keep an array of every command updated somewhere, but I feel like that would be a nonissue for the filters.

Ok then I'll remove it.

rob-3 commented 3 years ago

If you had something specific in mind, I'm not strongly against it or anything. For the uses I can think of though, dynamic imports seem like more trouble than they're worth.

suneettipirneni commented 3 years ago

If you had something specific in mind, I'm not strongly against it or anything. For the uses I can think of though, dynamic imports seem like more trouble than they're worth.

I don't I just pretty much added it to mirror commands, it commands are a different use case

suneettipirneni commented 3 years ago

Is this gtg?

rob-3 commented 3 years ago

Almost. I will move some of the code once the registerUI branch is finalized and merged, but no reason this can't go in now. Only thing I would suggest is a slight change in API by making runMessageFilters(message: Message, messageFilters: MessageFilter[]) into runMessageFilter(message: Message, filter: MessageFilter).