Command build rewrite
See documentation at: https://github.com/CommandAPI/docs/pull/2
Wow, I created this branch 1 year and 1 day ago (must have started working on the initial commit a bit before that). This PR rewrites the internal logic that turns CommandAPI commands into Brigadier and Bukkit commands, with the general goal being to fix issues, add features, and expand test coverage. Reading through all the commit descriptions is a decent way to see all the changes that were made, and I tried to write good comments for the new code, but I'll try to summarize things here. This PR relates to the following GitHub issues:
- #310 - Previous arguments are now properly accessible when generating suggestions after a redirect
- #470 - Argument permissions and requirements are now checked when the CommandAPI generates default help usage
- #483 - Adds a
FlagsArgumentfor Bukkit and Velocity - #513 - Adds a
DynamicMultiLiteralArgumentfor Bukkit and Velocity - #528 - Expanded API to allow dynamic help topics that depend on the player while also applying the CommandAPI's default formatting
- #536 - MultiLiteral usage now looks like
(java|bedrock), rather than being expanded into each literal path - #538 - Namespaced commands now have separate help topics to reflect how their unnamespaced version may be merged with another command.
- #557 - Fix converted command argument flattening when arguments contain spaces
- #559 - Players can now run Entity executors, restoring pre-9.0.0 behavior
This PR also addresses various oddities that I noticed while going through all the code. I believe these old behaviors are unwanted, though they evidently weren't enough of a problem to be discovered before and reported as an issue.
- Registering a
CommandTreewith no executions did not log any error - This now throws aMissingExecutorExceptionjust likeCommandAPICommand. - Unregistering a CommandAPI command now removes the corresponding entries from the list returned by
CommandAPI.getRegisteredCommands() - Removed some Bukkit-specific logic from
commandapi-coreAbstractCommandAPICommand.isConvertedand converted argument flattening logic- Permission registration moved to
CommandAPIBukkit#postCommandRegistration CommandMetaDataandRegisteredCommandOptional<Object> helpTopicreplaced byCommandAPIHelpTopicinterface and subclassBukkitHelpTopicWrapper- Split
ExecutorTypeenum for each platform. E.g. a Velocity command can no longer try to define an execution with typeExecutorType.ENTITY.
The largest changes come from refactoring the Brigadier node building logic out of CommandAPIHandler and into AbstractCommandAPICommand, AbstractCommandTree, AbstractArgumentTree, and AbstractArgument. This has the following implications:
- Argument subclasses can override the node building methods to define their own node structures
MultiLiterals share children nodes instead of creating duplicate command paths (seeArgumentMultiLiteralTests). This reduces the total amount of nodes in the tree.FlagsArgumentandDynamicMultiLiteralArgumentcan use custom CommandNode implementationsPreviewablearguments use a custom CommandNode to store the information needed to generate chat preview directly in the Brigadier tree, rather than in aMap<List<String>, Previewable<?, ?>> previewableArgumentsin CommandAPIHandler
CommandTrees, subcommands, and optional arguments don't need to be flattened into multipleCommandAPICommands. This theoretically reduces the amount of work that happens when registering these sorts of branching commands, as each argument should now only be processed once, though I haven't benchmarked anything.
While this PR is mostly internal changes, there are a few public-facing changes that make it backwards-incompatible:
- Removed
AbstractArgument#setOptional. As discussed here (https://github.com/JorelAli/CommandAPI/commit/193f237f9ca3cd65c659a695383ed3b9c388a111#r126281889), allowing unrestricted access to optional arguments inCommandTrees can create ambiguous situations. After further discussion in the CommandAPI discord (ongoings-development > Optional argument rewrite), it was resolved that optional arguments inCommandTrees are unnecessary when they can already have multiple executors defined. Optional arguments are still usable as before inCommandAPICommands using thewithOptionalArgumentsmethod. - Many changes to
RegisteredCommand(seeCommandAPICommandRegisteredCommandTestsandCommandTreeRegisteredCommandTestsfor examples in code)- Replaced
shortDescription,fullDescription, andusageDescriptionfields withCommandAPIHelpTopic - Currently, everything is flattened into a
CommandAPICommand, and each of those get their ownRegisteredCommandput into a list. With this PR, a singleRegisteredCommandobject gets created for each command literal node placed in the dispatcher. I.e. theMap<String, RegisteredCommand<CommandSender>> registeredCommandsinCommandAPIHandlershould roughly reflect the CommandAPI commands added to theMap<String, LiteralCommandNode<S>> childrenin the Brigadier dispatcher's root node. - The object that stores
RegisteredCommands inCommandAPIHandleris now a map. However, I didn't change the signature ofCommandAPI#getRegisteredCommands, so it is still only exposed as a list. - Previously, arguments were held in a
List<String>. Now, since aRegisteredCommandmay represent multiple command paths, an innerNoderecord is used to represent the Brigadier command tree. AList<List<String>> argsAsStrmethod was defined to provide the arguments in a similar format as before, which is still used when logging the commands being registered. TheNoderecord also allows accessing the permission and requirements of each Argument, which wasn't possible before.
- Replaced
- Many
executesmethods changesAbstractCommandSenderand all its subclass removed (no longer necessary to wrap command senders)- Sender specific executor classes replaced using generics. E.g.
PlayerExecutionInfo->NormalExecutorInfo<Player, ?> - These changes aren't a big deal when using lambdas. The public method signatures changed, so developer code needs to be recompiled, but no refactoring is needed by developers... Except when using non-DSL Kotlin, which has trouble inferring the type parameters for some reason. See the example files for a reference - Java and DSL examples didn't change much, but
Examples.ktdid.
At the moment, I believe I have finished all the code changes I intended and resolved all the TODOs I discovered along the way. Hence, I'm opening this PR for code review. Questions, comments, or concerns from anyone who has the time to look through all this or just a part is greatly appreciated! Implementations are always subject to review, and I haven't explained everything about the new code in this description, as I think it would be more useful to write things down in review comments where the code being addressed is more easily visible.