Add Kotlin DSL support for delegations by DerEchtePilz · Pull Request #482 · CommandAPI/CommandAPI · GitHub

As suggested by Sparky in the CommandAPI Discord, this PR is adding support for delegations in the Kotlin DSL.

This would allow retrieving arguments like this:

CommandAPICommand("cmd")
    .withArguments(StringArgument("string"))
    .executes(CommandExecutor { _, args -> 
        val string: String by args
    })
    .register()

Or, in the Kotlin DSL:

commandAPICommand("cmd") {
    stringArgument("string")
    anyExecutor { _, args -> 
        val string: String by args
    }
}

This PR restructures the Kotlin DSL modules which are now no longer included in the commandapi-bukkit and commandapi-velocity modules. Instead, the commandapi-kotlin module has been added to the root of the project which contains three modules: commandapi-core-kotlin, commandapi-bukkit-kotlin and commandapi-velocity-kotlin.
This results in this structure:

commandapi-core
commandapi-kotlin
- commandapi-core-kotlin
- commandapi-bukkit-kotlin
- commandapi-velocity-kotlin

To still be able to build all modules that belong to a specific platform, these modules have also been added to the commandapi-platforms pom.xml.

ToDo's:

Add an entry to the changelog
Write some documentation
- Add the documentation to kotlindsl.md
- Update documentation changelog in intro.md

commandapi-core

commandapi-kotlin

commandapi-core-kotlin

commandapi-bukkit-kotlin

commandapi-velocity-kotlin

Would it not make more sense to name them commandapi-kotlin-core, commandapi-kotlin-bukkit and commandapi-kotlin-velocity?

Would it not make more sense to name them commandapi-kotlin-core, commandapi-kotlin-bukkit and commandapi-kotlin-velocity?

Yes, I thought about this too. I thought if this is for 9.2.0 it might make more sense to leave the module names as they are because with this change a dependency change from commandapi-bukkit-kotlin to commandapi-kotlin-bukkit would be necessary and I am not sure that this is something we want for a minor version.

Yes, I thought about this too. I thought if this is for 9.2.0 it might make more sense to leave the module names as they are because with this change a dependency change from commandapi-bukkit-kotlin to commandapi-kotlin-bukkit would be necessary and I am not sure that this is something we want for a minor version.

Makes sense to me 👍

Just some notes about the pom.xml files.

I don't know enough to judge the Kotlin part, but it probably works. Maybe there could be some tests added for the feature.

Just some notes about the pom.xml files.

I don't know enough to judge the Kotlin part, but it probably works. Maybe there could be some tests added for the feature.

@willkroboth
This PR isn't done yet, I already planned to write tests and some documentation for it because it's also possible to use this while not having the Kotlin DSL modules commandapi-bukkit-kotlin and commandapi-velocity-kotlin as a dependency.

Thanks about the project version thingy though, I totally forgot that!

About the structure and the file paths. Given that it works I don't think the profile thing should be added to commandapi-kotlin. The goal of this was to have all the Kotlin stuff organized (hence the removal of commandapi-[platform]-kotlin from the commandapi-[platform] module) and I wanted no code repetition if possible which is why I added commandapi-kotlin in the first place.
Adding the profiles to commandapi-kotlin in my opinion defeats the last point as there would be essentially the same functionality at two places. The way it currently is makes it more clear what belongs to Platform.Bukkit and what belongs to Platform.Velocity, at least in my opinion.

I am also planning on doing some more testing with the modules to see how they behave when set up differently but that's nothing that I want to get too deep into now.

Just one comment on the tests.

I still think it makes sense to put

<modules>
	<module>commandapi-core-kotlin</module>
</modules>

<profiles>
	<profile>
		<id>Platform.Bukkit</id>
		<modules>
			<module>commandapi-bukkit-kotlin</module>
		</modules>
	</profile>
	<profile>
		<id>Platform.Velocity</id>
		<modules>
			<module>commandapi-velocity-kotlin</module>
		</modules>
	</profile>
</profiles>

into the pom.xml of commandapi-kotlin. If you want all the Kotlin stuff in the same place, that would keep all the Kotlin stuff inside the kotlin module instead of putting it in commandapi-platforms.

If you want it to be more clear what belongs to Bukkit/Velocity, you should put commandapi-bukkit-kotlin inside commandapi-bukkit's pom and commandapi-velocity-kotlin inside commandapi-velocity's pom, like it was before this PR. You do have to do the path traversal, which I think makes this slightly worse, but the purpose of the commandapi-[platform] modules are to store the platform-specific modules.

However, I suppose building the CommandAPI is functionally the same in any of these ways, so I'm not requesting changes for that anymore.

Looks good to me, to the extent that I can judge Kotlin code :P. I like the extra check that the exception message is right.

Still not sure about this: https://github.com/JorelAli/CommandAPI/pull/482/files#r1297831688, but oh well.

I'm guessing docs are on the TODO list? Oh wait, just noticed the PR description has a TODO list now, lol

…Kotlin's documentation for delegated properties

…le too

Add Kotlin DSL support for delegations

bf86f2b

Set scope to provided for commandapi-core dependency

c8ac59c

JorelAli approved these changes Aug 17, 2023

View reviewed changes

willkroboth requested changes Aug 17, 2023

View reviewed changes

Comment thread commandapi-kotlin/commandapi-bukkit-kotlin/pom.xml Outdated

Comment thread commandapi-kotlin/commandapi-core-kotlin/pom.xml Outdated

Comment thread commandapi-kotlin/commandapi-velocity-kotlin/pom.xml Outdated

Comment thread commandapi-platforms/pom.xml

DerEchtePilz added 2 commits August 22, 2023 14:10

Change version to use project.version

0c728a0

Adds tests for Kotlin DSL delegations

3f6ae42

DerEchtePilz requested a review from willkroboth August 22, 2023 12:46

willkroboth requested changes Aug 24, 2023

View reviewed changes

Comment thread ...api-bukkit-kotlin-test/src/test/kotlin/dev/jorel/commandapi/test/dsltests/DelegationTests.kt

Improve delegation test by checking for correct values (just to be sure)

40e2a64

DerEchtePilz requested a review from willkroboth August 24, 2023 15:33

willkroboth approved these changes Aug 25, 2023

View reviewed changes

DerEchtePilz added 7 commits September 21, 2023 22:27

Merge branch 'dev/dev' into dev/kotlindsl-delegations

3dafcf8

Update global changelog

c2a5cdd

Add documentation for Kotlin DSL delegations

a1898ad

Provide additional information about delegation usage

3bbc1c9

Merge branch 'dev/dev' into dev/kotlindsl-delegations

73d77b9

Fix MarkdownLint complaint

331d302

Fix new MarkdownLint complaint

c93b9ce

JorelAli reviewed Sep 22, 2023

View reviewed changes

Comment thread docssrc/src/kotlindsl.md Outdated

DerEchtePilz added 5 commits September 22, 2023 16:10

Change term 'delegated' to 'delegated properties'. Provide a link to …

3e08f47

…Kotlin's documentation for delegated properties

Remove Velocity dependency information in kotlinintro.md

a654182

Move delegated properties documentation to a new page

6992a42

Make delegated properties Kotlin example not use the Kotlin DSL examp…

0fb9cd2

…le too

Merge branch 'dev/dev' into dev/kotlindsl-delegations

c7eff6a

DerEchtePilz merged commit 9620fd3 into dev/dev Sep 23, 2023

Add Kotlin DSL support for delegations by DerEchtePilz · Pull Request #482 · CommandAPI/CommandAPI · GitHub

Uh oh!

Uh oh!

Add Kotlin DSL support for delegations - #482

Add Kotlin DSL support for delegations#482
DerEchtePilz merged 17 commits into
dev/devfrom
dev/kotlindsl-delegations

DerEchtePilz commented Aug 17, 2023 •
edited
Loading

Uh oh!

Uh oh!

JorelAli commented Aug 17, 2023

Uh oh!

DerEchtePilz commented Aug 17, 2023

Uh oh!

JorelAli commented Aug 17, 2023

Uh oh!

willkroboth left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DerEchtePilz commented Aug 17, 2023

Uh oh!

willkroboth left a comment

Uh oh!

Uh oh!

willkroboth left a comment •
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add Kotlin DSL support for delegations by DerEchtePilz · Pull Request #482 · CommandAPI/CommandAPI · GitHub

Uh oh!

Uh oh!

Add Kotlin DSL support for delegations - #482

Add Kotlin DSL support for delegations#482DerEchtePilz merged 17 commits intodev/devCommandAPI/CommandAPI:dev/devfrom dev/kotlindsl-delegationsCommandAPI/CommandAPI:dev/kotlindsl-delegationsCopy head branch name to clipboard

Conversation

DerEchtePilz commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JorelAli commented Aug 17, 2023

Uh oh!

DerEchtePilz commented Aug 17, 2023

Uh oh!

JorelAli commented Aug 17, 2023

Uh oh!

willkroboth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DerEchtePilz commented Aug 17, 2023

Uh oh!

willkroboth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willkroboth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add Kotlin DSL support for delegations#482
DerEchtePilz merged 17 commits into
dev/devfrom
dev/kotlindsl-delegations

DerEchtePilz commented Aug 17, 2023 •
edited
Loading

willkroboth left a comment •
edited
Loading