Implementation of ValidateAndGetBestMatch #1700
Conversation
This function will validate all commands from a SearchResult and return the result of said validation, along with the command matched, if a valid match was found.
quinchs
added
enhancement
priority: improvement
V3 Todo
|
Going to cc @Cenngo on this just for sanity |
| public async Task<(IResult, Optional<CommandMatch>)> ValidateAndGetBestMatch(SearchResult matches, ICommandContext context, IServiceProvider provider, MultiMatchHandling multiMatchHandling = MultiMatchHandling.Exception) | ||
| { | ||
| if (!matches.IsSuccess) | ||
| return (matches, Optional.Create<CommandMatch>()); |
There was a problem hiding this comment.
I dont think returning an optional CommandMatch to indicate to the user that their SearchResult wasnt successful isnt the most user friendly way of validating a command search. Instead of returning a Tuple, i think you should encapsulate this in an IResult that way you can provide more information to the user and it would be easier to build a command execution pipeline around this, since your goal is to allow people to utilize this.
| //Since ValidateAndGetBestMatch is deterministic on the return type, we can use pattern matching on the type for infering the code flow. | ||
| var (validationResult, commandMatch) = await ValidateAndGetBestMatch(searchResult, context, services, multiMatchHandling); | ||
|
|
||
| if(validationResult is SearchResult) |
There was a problem hiding this comment.
Tho i understand the way you are using this workflow to branch out the execution method, i dont think it is the most descript way of doing this. I think this should be more user readable to make it easier to maintain. Using an IResult to return the method result, with some additional info for branching out the pipeline, might be a better idea.
|
Other than that, it all seems good. |
|
@roridev Are you able to make the requested changes? |
|
Im able now, just give me a moment |
Changes: - Use IResult instead of Optional CommandMatch - Rework branching workflow
|
Please check if the changes done align with what was requested. Fun things that were discovered while doing this:
|
|
@Cenngo waiting on your final approval |
| return validationResult; | ||
| } | ||
|
|
||
| private async Task<IResult> handleCommandPipeline(MatchResult matchResult, ICommandContext context, IServiceProvider services) |
There was a problem hiding this comment.
Could you please make this pascal casing?
There was a problem hiding this comment.
oh sure rider autocorrected it to that.
As discussed on #1699 this pr comes with a rewrite of the logic on
CommandService#ExecuteAsync, splitting it into two functions.ExecuteAsync- will continue to do theSearch-Verify-Match-Executeloop as it did before.ValidateAndGetBestMatch- Verifies from a set of commands (SearchResult) with the specifiedICommandContext,IServiceProviderandMultiMatchHandlingwhich command is the optimal one, using the previously established classification and ranking systems.Breaking Changes
None.
Focus
Code cleanup and creation of an new public api.