Initial groundwork for command builders (#19) #20
Conversation
…ix invocation function
|
As it stands right now, all the supporting code (e.g. |
Co-authored-by: uwx <maxine@cybergal.com>
Co-authored-by: uwx <maxine@cybergal.com>
Co-authored-by: uwx <maxine@cybergal.com>
|
You've still got some unresolved comments on this PR. |
|
I can look into this later tonight, if not tomorrow. Hopefully this will be finalized by the end of the week, but if there's anything else that needs to be changed, I'm open to another round of review afterward! |
This fixes an issue where methods were not chainable because the return type was the abstract class instead of TSelf. The cast to TSelf is safe by virtue of being an abstract class, so this will always be a derived class.
This commit changes command merging to respect immutability; this comes at the cost of significantly more allocations which should be GC'd.
This commit fixes the merging algorithm by bringing it more in line with what ToChildNodes does. It also fixes an issue where top-level commands were not added to the root node.
This commit removes `IParameterShape#ParameterIndex` The rationale behind this is that the parameter was only added in the first place to ensure that parameters were parsed in order, however this is moot as the order of the command parameters is determined by the order they're added/created in, anyway, and thus ordering them is just unneccesary overhead. BREAKING-CHANGE: ParameterIndex was part of the public-facing API and has since been removed. Consumers should store an index when iterating over the command parameters, or use `.IndexOf(T)` on the collection.
VelvetToroyashi
force-pushed
the
feat/command-builders
branch
from
05c0b9b
to
30d2c4a
Compare
…/command-builders
|
Oops, just realized why the build was failing in CI. cc @Nihlus I believe I've resolved all the issues with this PR now. |
| /// </summary> | ||
| /// <param name="attribute">The attriubte to add.</param> | ||
| /// <returns>The builder to chain calls with.</returns> | ||
| public CommandParameterBuilder AddAttribute(Attribute attribute) |
There was a problem hiding this comment.
Might be nice to have an overload that takes a generic parameter here with a new() constraint - also lets us do compile-time checking for conditions that don't take any parameters (i.e, where TAttribute : ConditionAttribute and so on)
There was a problem hiding this comment.
Curious what kind of checks this would entail
| } | ||
|
|
||
| if (this.Min is null or 0) | ||
| else if (this.Min is null or 0) |
There was a problem hiding this comment.
Potentially redundant else - check.
|
Please rebase this onto the latest commit so you get all the required ReSharper inspection checks. |
This PR tracks the work of the described architectural changes described in #19.
This PR will close #19 and #2.
CommandNodeCommandNodeandGroupNodeCommandTreeBuilderto generate delegatesCommandTreeBuilderthat allows for adding runtime-defined commands