Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Commands API #55

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft

New Commands API #55

wants to merge 16 commits into from

Conversation

prokopyl
Copy link
Member

@prokopyl prokopyl commented Nov 18, 2020

(WIP Draft)

To-Do list:

  • Handle overloads with the same number of parameters of different types

    • By default, all overloads should be tried, by order of declaration, only first matching gets ran
    • Create @CommandMethod annotation with priority argument to change this
  • Handle fetching argument names:

    • Create @Param annotation with name argument
    • Fallback: use actual argument name (if available from compiler)
    • Fallback: use argument type name
  • Throw proper exception for missing argument(s) (for non-overriden methods)
    e.g. "Missing argument: /toaster add ",

  • Throw proper exception for extra argument(s) (for non-overriden methods)
    e.g. "Unexpected extra argument: /toaster add 5 foo", (foo in red)

  • Throw proper exception for invalid argument(s)
    e.g. "Invalid argument: /toaster add foo" (foo in red)
    " count: "foo" is not a number."

  • Standardize CommandException display formatting (using overridable methods such as "ErrorMessage", "Hint", etc.)

    • Add RawTextPart.then(RawTextPart other) helper method
    • Add RawText.join(Iterable<RawTextPart<?>> parts, RawTextPart separator) static helper method
  • Find a smart way to display error messages about missing/extra/invalid arguments when there are multiple overloads that mismatch for different reasons
    If multiple are found, order them using Levenshtein Distance?

  • Allow ArgumentType to consume multiple arguments (e.g. Location would consume "X Y Z")
    Add an override consuming an ArgumentList instead of String (default impl. forwards to the one consuming String
    Not consuming a single argument (without throwing) should be a hard error

  • Add an async ArgumentType variant (e.g. OfflinePlayer when a name is given and an API call has to be made to Mojang services)

    • Add an isFatal() overridable method on CommandException, e.g. for OfflinePlayer when the API call failed, so that a somewhat comprehensible error message can be sent to the user, but the error is still thrown to be registered by server logs or systems like Sentry
  • Add support for flags:

    • Flags should be parsed and collected separately from positional arguments, and are optional by default
    • Create @Flag annotation with short and long names (at least one should be specified)
    • Supported syntaxes: -f, -f=42, --flag, --flag=42
    • Supported types: boolean, Optional
      • Throw runtime exception when not any of those types
    • Throw proper exception when a flag is set multiple times
    • Throw proper exception when a flag has no value set but is required to have one
    • Throw proper exception when a flag has a value set but shouldn't have one
  • Add support for multiple-flags:

    • @Flag(multiple = true)
    • Supported types: boolean, int, Optional<T[]>
      • Throw runtime exception when not any of those types
  • Add support for required flags (can be multiple as well):

    • @Flag(required = true)
    • Supported types: T
      • Throw runtime exception when not any of those types
    • Throw proper exception when required flag is missing
  • Add native type handlers (more can be added):

    • Java types:
      • byte, short, int, long
      • float, double
      • T[]
      • UUID
      • Locale
    • Bukkit types:
      • NamespacedKey
      • Material (based on NamespacedKey)
      • Player (based on name or UUID)
      • OfflinePlayer (async)
      • World (based on NamespacedKey)
      • Location (optionally based on World)

@prokopyl prokopyl added C ⋅ Commands Component – Commands API T ⋅ Feature Type – Feature Request labels Nov 18, 2020
@prokopyl prokopyl self-assigned this Nov 18, 2020
@prokopyl prokopyl mentioned this pull request Nov 18, 2020
@AmauryCarrade
Copy link
Member

Est-ce que les commandes auront une méthode “racine” ?

Il serait pratique de pouvoir créer des commandes racines (par exemple /command) sans passer par un alias de sous-commande. Avec le gestionnaire actuel de commandes de QuartzLib, on rencontre souvent des cas où des sous-commandes sont crées uniquement pour être des commandes principales (par exemple, /maptool new, /maptool explore, et toutes les commandes de /voteban…). Comment serais-ce géré ?

@prokopyl
Copy link
Member Author

Est-ce que les commandes auront une méthode “racine” ?

Il serait pratique de pouvoir créer des commandes racines (par exemple /command) sans passer par un alias de sous-commande. Avec le gestionnaire actuel de commandes de QuartzLib, on rencontre souvent des cas où des sous-commandes sont crées uniquement pour être des commandes principales (par exemple, /maptool new, /maptool explore, et toutes les commandes de /voteban…). Comment serais-ce géré ?

Oui, je pensais avoir une annotation @Main (ou similaire) sur l'une des méthodes de la classe qui permette de faire ça.

@AmauryCarrade
Copy link
Member

AmauryCarrade commented Nov 20, 2020

Autre remarque. Peut-on imaginer supporter d'une façon ou d'une autre (p.ex. une annotation @MultiWords ou une configuration de la commande, ou même quelque chose d'universel) le fait d'avoir des arguments entre guillemets pour qu'ils puissent contenir des espaces ?

Par exemple, il serait cool que quelque chose comme ceci :

@Main
public void toast(String toast) {
    assertEquals(toast, "Je suis un grille pain");
}

fonctionne avec cet appel.

/cmd "Je suis un grille pain"

Ce genre de choses ont le don de rendre l'autocomplétion parfois tricky, cela dit (enfin, il faut analyser la ligne de commande pour voir si on est dans une chaîne en cours ou pas, quoi).

@prokopyl
Copy link
Member Author

Pour moi ça devrait être le comportement standard (sans devoir ajouter une annotation), vu que c'est la syntaxe standard des commandes il me semble maintenant.

Pour parser la commande aussi je pensais utiliser le nouveau parseur officiel de Minecraft : https://github.com/Mojang/brigadier , comme ça on sera 100% standard et on pourra utiliser les @p[...] et autres dans les commandes ^^

@AmauryCarrade
Copy link
Member

J'ignorais que Brigadier gérait ça ; parfait alors ! Ce serait d'autant mieux, l'absence de support des @p et autres est très handicapante généralement. Ça permettra aussi une série de simplifications…

@prokopyl prokopyl force-pushed the new-commands branch 2 times, most recently from 85290da to 02f02dd Compare December 17, 2020 15:33
Copy link
Member

@AmauryCarrade AmauryCarrade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments about the unknown command output (great idea by the way!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C ⋅ Commands Component – Commands API T ⋅ Feature Type – Feature Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants