Skip to content

Implement Type-safe Kotlin DSL#545

Closed
sya-ri wants to merge 1 commit intoCommandAPI:dev/devfrom
sya-ri:dev/type-safe-kotlin-dsl
Closed

Implement Type-safe Kotlin DSL#545
sya-ri wants to merge 1 commit intoCommandAPI:dev/devfrom
sya-ri:dev/type-safe-kotlin-dsl

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
Member

@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
Member

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
Member

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
Member

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
Member

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
@sya-ri sya-ri closed this Apr 29, 2025
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.

3 participants