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

Implement Type-safe Kotlin DSL #545

Open
wants to merge 1 commit into
base: dev/dev
Choose a base branch
from

Conversation

sya-ri
Copy link

@sya-ri sya-ri commented Apr 16, 2024

#544


I didn't change the documentation because I didn't really understand it.

Copy link
Collaborator

@DerEchtePilz DerEchtePilz left a comment

Choose a reason for hiding this comment

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

Again, this should not be a replacement. This should rather be an alternative for those who want this.
Please also remove the additional methods containing the Optional part, especially in the CommandTreeDSL because as mentioned in your issue, optional arguments are going to be removed from CommandTrees in the future. Regardless, these methods are not wanted in the CommandAPICommandDSL either.

Other than that I honestly don't really understand how this works exactly so I definitely want to test this myself first, but that's obviously my problem :D

@DerEchtePilz
Copy link
Collaborator

I didn't change the documentation because I didn't really understand it.

The documentation is built using mdbook, I believe there are download links for the required executables in the wiki here on GitHub.
The source for the documentation consists of .md files. Those can be found in docssrc/src.
The documentation additionally uses compiled code so the code examples can be used in code without paying attention to the correct syntax. Its source resides in the commandapi-documentation-code. This module is the Bukkit module, there is not really documentation for Velocity yet.

@sya-ri
Copy link
Author

sya-ri commented Apr 16, 2024

Again, this should not be a replacement. This should rather be an alternative for those who want this.

Does that mean I should leave the method's arguments and processing as they are rather than changing them, and define new methods in another file?

The only incompatibility caused by this replacement is when specifying argument(..., optional = false) or argument(..., optional = true); you should remove optional = false or use optionalArgument(...).

Retrieval methods using get and cast are still supported. I just made it possible to use the getter inside the lambda.

https://github.com/JorelAli/CommandAPI/pull/545/files#diff-a396609c4f42dbc82e492e9976de00ceb4c97805935fc741902fbb6cb4eae000
https://github.com/JorelAli/CommandAPI/pull/545/files#diff-67d7c60b582802c1efad0034b5b7bf5da58e284935c795f2959d5aa5bab2e654
https://github.com/JorelAli/CommandAPI/pull/545/files#diff-db52ae2a394c8abc971e1c4845fb833df06243d1096b6e8938dd042ed4eead99

I only changed the optional tests and added some tests.

@DerEchtePilz
Copy link
Collaborator

Does that mean I should leave the method's arguments and processing as they are rather than changing them, and define new methods in another file?

Yeah, that would be ideal.

The only incompatibility caused by this replacement is when specifying argument(..., optional = false) or argument(..., optional = true); you should remove optional = false or use optionalArgument(...).

I mean, technically you can do whatever here, just don't introduce duplicate methods like the ones that have the Optional in their name. That is something we definitely don't want.

@DerEchtePilz DerEchtePilz self-assigned this Apr 16, 2024
@DerEchtePilz
Copy link
Collaborator

Hmm, I just realized, we do have Delegated properties support for the Kotlin DSL.

That of course doesn't prevent this feature from being added but generally it is sort of the same functionality, right?

@willkroboth
Copy link
Collaborator

I think delegated properties are still syntactic sugar for CommandArguments#getUnchecked. You could still accidentally put the wrong class, and you wouldn't know until runtime when there's a ClassCastException or something.

commandTree("sendmessageto") {
    playerArgument("player") {
        greedyStringArgument("msg") {
            anyExecutor { _, args ->
                val player: String by args // At runtime: Inconvertible types, Player is not a String
                val message: String by args
                player.sendMessage(message)
            }
        }
    }
}

The changes here allow there to be a compile time exception. IDEs can also statically analyze the code and catch the error, which makes it easier for the developer to notice and fix.

commandTree("sendmessageto") {
    playerArgument("player") { getPlayer ->
        greedyStringArgument("msg") { getMessage ->
            anyExecutor { _, args ->
                String player = getPlayer(args) // At compile time: Inconvertible types, Player is not a String
                val message = getMessage(args)
                player.sendMessage(message)
            }
        }
    }
}

@DerEchtePilz
Copy link
Collaborator

Yeah, true. I mean, it also depends on the variable name you put there so the type safety is still only sort of there.
I mean yeah, in the end a call to CommandArguments#get(String) as T is done because the unsafe CommandArguments#getUnchecked wouldn't exactly work here (as we don't exactly work with fixed types there I think)

@willkroboth willkroboth mentioned this pull request Apr 20, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants