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

Implementing optional arguments #393

Merged
merged 28 commits into from
Jan 4, 2023
Merged

Conversation

DerEchtePilz
Copy link
Collaborator

@DerEchtePilz DerEchtePilz commented Jan 3, 2023

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:

new CommandAPICommand("optionalarguments")
    .withArguments(new PlayerArgument("player"))
    .withArguments(new ItemStackArgument("item"))
    .withOptionalArguments(new IntegerArgument("amount"))
    .withOptionalArguments(new BooleanArgument("broadcast"))
    .executesPlayer(info -> {
        Player target = (Player) info.args().get("player");
        ItemStack item = (ItemStack) info.args().get("item");
        Integer amount = (Integer) info.args().get("amount");
        Boolean broadcast = (Boolean) info.args().get("broadcast");
        if (amount != null) {
            item.setAmount(amount);
        }
        target.getInventory().addItem(item);
        if (broadcast == null) {
            info.sender().sendMessage("You gave " + target.getName() + " " + item.getType().name().toLowerCase());
        } else {
            Bukkit.broadcastMessage(target.getName() + " has received " + item.getType().name());
        }
    })
    .register();

When executing this command in Minecraft, you have those options (for example):

/optionalargument DerEchtePilz minecraft:netherite_block
/optionalargument DerEchtePilz minecraft:netherite_block 5
/optionalargument DerEchtePilz minecraft:netherite_block 5 true

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:

  • Documentation?
  • Address code reviews
  • Add this to the Kotlin DSL
  • Add missing JavaDocs

Copy link
Owner

@JorelAli JorelAli left a comment

Choose a reason for hiding this comment

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

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:

    • One extra method call
    • One null check
    • One bootstrap lambda added to the Java class file (increases the file size)
    • One additional method call for each iteration because it has to call the lambda
    • Literally does a for-each loop under the hood
  • I really like the renaming of args to arguments and argumentsArr to argumentsArray, thanks!

Copy link
Owner

@JorelAli JorelAli left a comment

Choose a reason for hiding this comment

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

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.

@DerEchtePilz
Copy link
Collaborator Author

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.

  • So, we remove the section Optional/Different Arguments from arguments.md?
  • We are creating a new page optional_arguments.md?

@JorelAli
Copy link
Owner

JorelAli commented Jan 4, 2023

  • So, we remove the section Optional/Different Arguments from arguments.md?

Yes 👍

  • We are creating a new page optional_arguments.md?

Yes 👍

@DerEchtePilz DerEchtePilz marked this pull request as ready for review January 4, 2023 14:13
@JorelAli
Copy link
Owner

JorelAli commented Jan 4, 2023

In #360, there was an idea of being able to retrieve "default" values from optional arguments:

It is useful to be able to access arguments by node name, rather than by index - especially in the case of using optional arguments:

(CommandSender sender, Map<String, Object> args) -> {
    String input = (String) args.get("blah");
    int amount = (int) args.getOrDefault("amount", 1);
}

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.

@DerEchtePilz
Copy link
Collaborator Author

You need to help me with the Supplier part though. I am not too familiar with them.
Would I implement them like this:

public Object getOrDefault(int index, Supplier<?> defaultValue)

@JorelAli
Copy link
Owner

JorelAli commented Jan 4, 2023

You need to help me with the Supplier part though. I am not too familiar with them. Would I implement them like this:

public Object getOrDefault(int index, Supplier<?> defaultValue)

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().

@DerEchtePilz DerEchtePilz marked this pull request as draft January 4, 2023 16:13
@DerEchtePilz DerEchtePilz marked this pull request as ready for review January 4, 2023 16:32
Copy link
Owner

@JorelAli JorelAli left a comment

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants