| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…kkit into commandapi-bukkit-kotlin. Rename commandapi-kotlin-velocity into commandapi-vvelocity-kotlin
There was a problem hiding this comment.
Initial code review. Main review points:
Avoid list.forEach() in favour of a for-each loop. Having the mindset of using forEach() with lambdas instead of existing features (for loops) all the time is not desirable, and has in fact degraded the quality of the code you've written. Using forEach() always adds unnecessary performance overhead compared to a for-each loop:
I really like the renaming of args to arguments and argumentsArr to argumentsArray, thanks!
Sorry, something went wrong.
There was a problem hiding this comment.
The documentation content looks good, just a few minor changes with the main content.
I think showcasing optional arguments would be much better on its own page instead of at the bottom of arguments.md. Can we please create a new page (e.g. optional_arguments.md), and place that above the "Listed arguments" section in SUMMARY.md.
The new page should be titled "Optional arguments" (get rid of "/Different" from the title, I don't want that anymore). I'd like some explanation about optional arguments and a block of code at the top showing the various withOptionalArguments() methods.
If you're struggling to meet these requirements, just let me know and I'll help out.
Sorry, something went wrong.
|
Sorry, something went wrong.
Yes 👍
Yes 👍 |
Sorry, something went wrong.
…ptional_arguments.md
|
In #360, there was an idea of being able to retrieve "default" values from optional arguments:
Since we're adding all of the optional argument stuff now, it may be worth considering adding this to this PR. I think it's a good idea to add this to simplify null checks for arguments you know are going to be optional arguments: new CommandAPICommand("sayhi")
.withOptionalArguments(new PlayerArgument("target"))
.executesPlayer((player, args) -> {
Player target = (Player) args.getOrDefault("target", player);
target.sendMessage("Hi!");
})
.register();It may also be worth considering implementing a Supplier<?> overload as well: new CommandAPICommand("sayhi")
.withOptionalArguments(new PlayerArgument("target"))
.executesPlayer((player, args) -> {
Player target = (Player) args.getOrDefault("target", () -> getBestFriend(player));
target.sendMessage("Hi!");
})
.register();This could also safely handle the index overload without an index out of bounds exception. |
Sorry, something went wrong.
|
You need to help me with the Supplier part though. I am not too familiar with them. public Object getOrDefault(int index, Supplier<?> defaultValue) |
Sorry, something went wrong.
Yes, that's precisely it. A Supplier<T> is effectively identical to Function<Void, T> - it's a function that doesn't have any input parameters, but returns a value. You can call the supplier using supplier.get(). |
Sorry, something went wrong.
There was a problem hiding this comment.
In CommandArguments, the following method can throw an ArrayIndexOutOfBoundsException:
public Object get(int index) {
return args[index];
}With the introduction of optional arguments, users are now more likely to access arguments via indexes that are out of bounds. To retain feature parity with get(String nodeName), please make get(int index) return null when getting an argument by position that doesn't exist. As expected, please make sure this method has @Nullable on it.
Sorry, something went wrong.
…ption, but instead let it return null
| Back | FazBrowse Home | New Git URL |
This PR adds optional arguments to the CommandAPI!
This has been a topic that was not only discussed serveral times on the CommandAPI discord but also exists as an issue on GitHub (#162).
This PR adds the withOptionalArguments method that allows users to implement, well, optional arguments.
Take this code for example:
When executing this command in Minecraft, you have those options (for example):
The first option puts You gave DerEchtePilz netherite_block in the chat and I receive one item
The second option puts You gave DerEchtePilz netherite_block in the chat but I receive five items
The third option puts DerEchtePilz has received NETHERITE_BLOCK in the chat and I receive five items
Things to add before merging: