NetCordDev / NetCord

The modern and fully customizable C# Discord library.
https://netcord.dev
MIT License
65 stars 10 forks source link

Result handling logic should be expanded to support class-based handlers. #34

Closed csmir closed 2 months ago

csmir commented 2 months ago

Description

As discussed on the Discord, it is the intent to implement class-based result handlers for every service, in pursuit of more accessible and customizable result handling. The following naming suggestions apply:

These classes should be optional and not used by default. The original result handling logic can be retained, and the service provider can include the discovered objects on module creation, to be fetched at post-execution. (Can simply be done in HandleResultAsync default overload)

Unclear:

Do we want to support these types to be singleton or transient through something like an attribute? (scoped is irrelevant, it would adopt same logic as transient), or will we simply force either of the two on the recipient?

Red-K0 commented 2 months ago

Might be better to use a ResultHandlers namespace or something similar, just to shorten the names in this case.

KubaZ2 commented 2 months ago

How would types in ResultHandlers be named?

KubaZ2 commented 2 months ago

A footnote here, should this include ...Text... for clarity or take from preexisting naming?

I think it should be named ICommandResultHandler, without the Text

csmir commented 2 months ago

Might be better to use a ResultHandlers namespace or something similar, just to shorten the names in this case.

You would not write these names more than once in any project, and they would be best implemented in the Netcord.Hosting.Services project. Besides, the type clarity would crumble if ResultHandler is excluded, intellisense does not point out the namespace a type is from when it is imported

KubaZ2 commented 2 months ago

Do we want to support these types to be singleton or transient through something like an attribute? (scoped is irrelevant, it would adopt same logic as transient), or will we simply force either of the two on the recipient?

I think there are no pros of using transient in this case. Are there? Also could you clarify how would the attributes work?

KubaZ2 commented 2 months ago

You would not write these names more than once in any project, and they would be best implemented in the Netcord.Hosting.Services project. Besides, the type clarity would crumble if ResultHandler is excluded, intellisense does not point out the namespace a type is from when it is imported

Yeah, I don't think it's a good idea, but maybe @Red-K0 has some better idea for the names we don't know about.

Red-K0 commented 2 months ago

Might be better to use a ResultHandlers namespace or something similar, just to shorten the names in this case.

You would not write these names more than once in any project, and they would be best implemented in the Netcord.Hosting.Services project. Besides, the type clarity would crumble if ResultHandler is excluded, intellisense does not point out the namespace a type is from when it is imported

Intellisense (at least on VS) does show what namespace a class is from if it would need to add a using or package to use it. As for name complexity, there's no realistic reason to be so verbose about the handler names. ICommandResultHandler can just be ResultHandlers.CommandResult or something. I'm sure there's better, just nothing coming directly to mind at the moment.

csmir commented 2 months ago

I honestly disagree with this, it makes things more confusing than they have to be. Note, I did already say 'When they are imported' in regards to revealing namespaces.

Red-K0 commented 2 months ago

I honestly disagree with this, it makes things more confusing than they have to be. Note, I did already say 'When they are imported' in regards to revealing namespaces.

Ah missed your last statement, my bad. Regardless it is just an idea, I'm mostly just concerned about the class' name length, but if it'll only ever be used once or twice then it should be fine.