Rework command result logic

[FiniteReality]

I'll use this issue to track the status of result logic in commands. Please discuss changes that you would like to be made here.

• Use classes instead of structs for results to allow for polymorphism
• Refactor result types (especially type readers) to make their public API cleaner
• Improve overall use-cases for result types
• Remove methods from converting one result to another
• Remove processing of result types in our info classes

[rikimaru0345]

Why do TypeReaders implement IResult?
After some investigation I can see that it could be helpful to know when/if a TypeReader failed, but are there any real-world scenarios where this is the only way to solve this?
I think that maybe type readers should get their own interface.
What current use cases exist where there's need to get a TypeReaderResult from the command service?
And couldn't those scenarios be solved differently?

If we want to improve the general use-case for result types we have to ask how they are currently used and what (real!) use-cases we could imagine / foresee in the future,
and then go from there. (Thinking how those specific scenarios could be solved best, and how result types fit into that, if at all!)

I think changing from struct to class is pretty straightforward in terms of "should we or should we not".
The only thing that we have to keep in mind there is:
a) What would returning null mean exactly? We should handle that case somehow and give a solid definition of what it means.
Maybe it could be interpreted as "success" and if there's need the command service would offer an .ExecutedCommand property or something?
b) We should encourage* people to cache return types. If possible every user should have something like static IResult successResult = ... somewhere to always return instead of new-ing up a new instance all the time for no reason at all.

[FiniteReality]

Why do TypeReaders implement IResult?

TypeReaders use IResult because they are returned as part of ExecuteAsync, which returns an IResult. As for returning a TypeReaderResult, it's often nice to be able to see why an overload failed to parse. (e.g. passing an invalid channel in ChannelTypeReader)

If users return null, I believe it would be best to let them handle it since I want to modify result types to not take other IResults to clean up our code a bit, so we won't be doing any processing of IResults except in ExecuteAsync.

I agree with caching return types too, but instead of caching them in their type reader, we should instead provide a static property on the default type readers for successful results.

[MarkusG]

I believe there was talk about this before, but perhaps allow for commands to return Task<ExecuteResult> to allow for custom execution results? Would also require a slight modification to the CommandError enum.

[FiniteReality]

I believe there was talk about this before, but perhaps allow for commands to return Task<ExecuteResult> to allow for custom execution results? Would also require a slight modification to the CommandError enum.

Yes, this was being handled in #466 but I believe the general consensus from Volt/Fox was that it wasn't really important or necessary. If you'd like to convince them otherwise, feel free to give a solid use case for the feature.

[Joe4evr]

b) We should encourage people to cache return types. [...]

Good luck, we've been encouraging users to do a lot of things, to very little avail. Switching result types to classes now would cause PEBKAC user reports until we get analyzers shipped with the lib itself.

a) What would returning null mean exactly?

Another reason why I feel we shouldn't do this just yet. At the very least, this one could be solved if the C# Language team manages to implement their "nullable reference types" static analysis sooner rather than later.

[FiniteReality]

b) We should encourage people to cache return types. [...]

Good luck, we've been encouraging users to do a lot of things, to very little avail. Switching result types to classes now would cause PEBKAC user reports until we get analyzers shipped with the lib itself.

This actually isn't that hard: for results that need no arguments, we can provide static members (e.g. ExecuteResult.Success) which should actually make things simpler for users.

a) What would returning null mean exactly?

Another reason why I feel we shouldn't do this just yet. At the very least, this one could be solved if the C# Language team manages to implement their "nullable reference types" static analysis sooner rather than later.

I'm currently leaning on a "treat null returns as an unknown error" approach because it makes our code easier, and I haven't really seen any reason why I shouldn't do it this way.

[Joe4evr]

This actually isn't that hard: for results that need no arguments, we can provide static members

After a small experiment, this can work, even with polymorphism in the mix. The hard part at that point is needing to tell users to supply their own static member if they were to sub-class an existing result type.

I'm currently leaning on a "treat null returns as an unknown error" approach

Okay, that's fair.