CommandAPI icon indicating copy to clipboard operation
CommandAPI copied to clipboard

Command build rewrite

Open willkroboth opened this issue 1 year ago • 0 comments

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 FlagsArgument for Bukkit and Velocity
  • #513 - Adds a DynamicMultiLiteralArgument for 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 CommandTree with no executions did not log any error - This now throws a MissingExecutorException just like CommandAPICommand.
  • Unregistering a CommandAPI command now removes the corresponding entries from the list returned by CommandAPI.getRegisteredCommands()
  • Removed some Bukkit-specific logic from commandapi-core
    • AbstractCommandAPICommand.isConverted and converted argument flattening logic
    • Permission registration moved to CommandAPIBukkit#postCommandRegistration
    • CommandMetaData and RegisteredCommand Optional<Object> helpTopic replaced by CommandAPIHelpTopic interface and subclass BukkitHelpTopicWrapper
    • Split ExecutorType enum for each platform. E.g. a Velocity command can no longer try to define an execution with type ExecutorType.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 (see ArgumentMultiLiteralTests). This reduces the total amount of nodes in the tree.
    • FlagsArgument and DynamicMultiLiteralArgument can use custom CommandNode implementations
    • Previewable arguments use a custom CommandNode to store the information needed to generate chat preview directly in the Brigadier tree, rather than in a Map<List<String>, Previewable<?, ?>> previewableArguments in CommandAPIHandler
  • CommandTrees, subcommands, and optional arguments don't need to be flattened into multiple CommandAPICommands. 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 in CommandTrees can create ambiguous situations. After further discussion in the CommandAPI discord (ongoings-development > Optional argument rewrite), it was resolved that optional arguments in CommandTrees are unnecessary when they can already have multiple executors defined. Optional arguments are still usable as before in CommandAPICommands using the withOptionalArguments method.
  • Many changes to RegisteredCommand (see CommandAPICommandRegisteredCommandTests and CommandTreeRegisteredCommandTests for examples in code)
    • Replaced shortDescription, fullDescription, and usageDescription fields with CommandAPIHelpTopic
    • Currently, everything is flattened into a CommandAPICommand, and each of those get their own RegisteredCommand put into a list. With this PR, a single RegisteredCommand object gets created for each command literal node placed in the dispatcher. I.e. the Map<String, RegisteredCommand<CommandSender>> registeredCommands in CommandAPIHandler should roughly reflect the CommandAPI commands added to the Map<String, LiteralCommandNode<S>> children in the Brigadier dispatcher's root node.
    • The object that stores RegisteredCommands in CommandAPIHandler is now a map. However, I didn't change the signature of CommandAPI#getRegisteredCommands, so it is still only exposed as a list.
    • Previously, arguments were held in a List<String>. Now, since a RegisteredCommand may represent multiple command paths, an inner Node record is used to represent the Brigadier command tree. A List<List<String>> argsAsStr method was defined to provide the arguments in a similar format as before, which is still used when logging the commands being registered. The Node record also allows accessing the permission and requirements of each Argument, which wasn't possible before.
  • Many executes methods changes
    • AbstractCommandSender and 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.kt did.

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.

willkroboth avatar Sep 03 '24 22:09 willkroboth