Velocity
Velocity copied to clipboard
PaperMC
Stabilize and expose suggestions API
This PR stabilizes and exposes the internal suggestions API for third-party developers. My usecase for this API is that I'm building a dashboard for running commands on both bukkit and velocity servers with tab completion. Right now there is no API to get any command completions for Velocity commands using the API. Bukkit already has a public API for tab completions: https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/command/CommandMap.html#tabComplete(org.bukkit.command.CommandSender,java.lang.String) So I think it would be good if velocity also exposed the internal suggestions API.
Could you point to a method where I can get the underlying brigadier dispatcher? This PR is not for writing commands, it's for tab completing any command virtually on the proxy for external use. Bukkit already has an API and Velocity has too, but it's internal. I'm making it public with this PR.
Right now the only way to get command completions for commands to external services like Web dashboards is through reflection. But internally there is already an API. This PR exposes that API publicly.
Like you said, it is already usable with reflection. It is internal for a reason and not exposed, as it may change in the future.
Well will it ever become stable or public? I don't understand the timeline on this. I haven't seen any other PR introducing a public suggestions API, so I made this one.
If I were to open a PR where I make custom code for this for a public API, I am 100% sure reviewers would say "we already have this API internally, just make the methods public via the API".
If I may ask, what is the reason it was kept internal and no public API was made? I assumed just noone bothered to stabilize it, so I made this PR.
Well will it ever become stable or public? I don't understand the timeline on this. I haven't seen any other PR introducing a public suggestions API, so I made this one.
I'm saying that it is internal and intentionally not exposed because there is no real reason for it to be in the API. As for third-party services, you can still use reflection to access it if needed. In 'normal' usage, Velocity has no reason to parse commands itself, as that is handled by Brigadier.
I don't think a useful feature that already has methods implemented should be only accessible using reflection... Reflection may break in future releases. Velocity has those methods to interact with the brigadier dispatcher internally for suggestions I assume? I'm just making it public with this PR to allow plugins for usecases like mine to not have to rely on reflection for this.
I don't think a useful feature that already has methods implemented should be only accessible using reflection... Reflection may break in future releases. Velocity has those methods to interact with the brigadier dispatcher internally for suggestions I assume? I'm just making it public with this PR to allow plugins for usecases like mine to not have to rely on reflection for this.
Yes, and I'm saying that it should be kept internal because you normally shouldn't need to use it. I understand that reflection can break, and that's what I'm trying to say. If internal APIs are used in plugins, it should be the responsibility of the plugin developers to update their plugins, rather than placing the burden on the maintainers.
Well what does normally mean? I have a totally normal plugin as anyone else. I already can execute commands using the executeAsync API, I just want to also tab complete commands using offerSuggestions. This isn't some crazy obscure feature. Even Bukkit has it. What's not normal about this?
Well what does normally mean? I have a totally normal plugin as anyone else. I already can execute commands using the executeAsync API, I just want to also tab complete commands using offerSuggestions. This isn't some crazy obscure feature. Even Bukkit has it. What's not normal about this?
It is "obscure" because command handling is completely done internally. Also, Velocity has nothing to do with other software.
Well command handling is also done by plugins that register RawCommands. I don't get why this shouldn't be a feature... You will also find similar features in both BungeeCord API: https://github.com/SpigotMC/BungeeCord/blob/cd56fb32c207b39b9470d66d7c61f68d9f0c7e78/api/src/main/java/net/md_5/bungee/api/plugin/PluginManager.java#L213 and also in Sponge API: https://jd.spongepowered.org/spongeapi/12.0.0-SNAPSHOT/org/spongepowered/api/command/manager/CommandManager.html#complete(java.lang.String)
There's nothing wrong about looking at other software and seeing that it's totally normal for plugins of their platform to use this API. I don't see why Velocity needs to be an exception to this. Brigadier command registration itself is also fully exposed by Velocity.
Well command handling is also done by plugins that register RawCommands. I don't get why this shouldn't be a feature... You will also find similar features in both BungeeCord API: https://github.com/SpigotMC/BungeeCord/blob/cd56fb32c207b39b9470d66d7c61f68d9f0c7e78/api/src/main/java/net/md_5/bungee/api/plugin/PluginManager.java#L213 and also in Sponge API: https://jd.spongepowered.org/spongeapi/12.0.0-SNAPSHOT/org/spongepowered/api/command/manager/CommandManager.html#complete(java.lang.String)
There's nothing wrong about looking at other software and seeing that it's totally normal for plugins of their platform to use this API. I don't see why Velocity needs to be an exception to this. Brigadier command registration itself is also fully exposed by Velocity.
These APIs were designed for the pre-1.13 command system, which Velocity was not. It is meant to be a "modern" API, supporting the Brigadier-based command system that handles commands internally. Your use case is not a typical plugin that has any in-game effect, so I don't understand why it should be supported.
That it's pre-1.13 has nothing to do with the API itself. Sponge for example would've removed it as they don't support 1.12.2 or below. You can even see Sponge updated their API to allow brigadier suggestions. This PR also uses post-1.13 tab completions as it directly returns CompletableFuture< Suggestions >. The Velocity API I'm making public with this PR is just a light wrapper on top of the brigadier command dispatcher as Velocity doesn't allow getting the brigadier dispatcher outside of the CommandManager., not even for internal code. Are you saying you would prefer if velocity instead returned a custom wrapper classes like sponge for command completions rather than brigadier classes directly?
No, I'm saying
Your use case is not a typical plugin that has any in-game effect, so I don't understand why it should be supported.
There are many plugins for velocity that don't have in-game effects. Take bungee-prometheus-exporter as an example: https://github.com/weihao/bungeecord-prometheus-exporter Or any discord bridge plugin that runs on velocity. I know not every plugin needs this API, but no plugin needs to use all powerful APIs Velocity has.
My plugin may not be the "average" plugin, but I still think this is a feature Velocity should offer for usecases like mine. That's why so many other platforms offer this API, to allow usecases like mine.
A Discord bridge plugin has an in-game effect, and as I see it, the "bungee-prometheus-exporter" is not a typical plugin in that it doesn't directly have an in-game effect. I don't want to say that every plugin should use the API or anything, but I don't understand why something like this should be exposed, as it may just be another burden when changing internals. I'm not a maintainer; I just mean that it has only a minor use case. However, since it is a very minor addition as well, I may just be exaggerating this. I've just seen some features that were not added to the API because they were unsupported.
Well then the maintainers can decide on this. I am fine if this API won't be "stable". But I'd like to ask for this to be included and annotated as @APIStatus.Internal as a minimum. Then I can actually get compilation errors when velocity changes the method signature.
Well then the maintainers can decide on this. I am fine if this API won't be "stable". But I'd like to ask for this to be included and annotated as @APIStatus.Internal as a minimum. Then I can actually get compilation errors when velocity changes the method signature.
If it isn't stable or new API, the API is typically annotated with @Beta, but as far as I know, internal API is never exposed.
Well ig the maintainers can decide... Maybe this is worth beta. But APIStatus.Internal would be fitting. This is not a very big API to expose.
Here a link to a relevant Discord discussion about this PR: https://discord.com/channels/289587909051416579/908507886420910101/1273240566171435128
@4drian3d sorry for the delay, I've now added the javadocs you requested. I don't want this to be reflection-only API because that can break with new versions, this doesn't.
@4drian3d Hi, could you look into getting this PR merged? It's rather simple and has been open for roughly a year.