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

Add Kotlin DSL support for delegations#482

Merged
DerEchtePilz merged 17 commits into
dev/devfrom
dev/kotlindsl-delegations
Sep 23, 2023
Merged

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

Conversation

@DerEchtePilz

@DerEchtePilz DerEchtePilz commented Aug 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

@JorelAli

JorelAli commented Aug 17, 2023

Copy link
Copy Markdown
Member

@DerEchtePilz

DerEchtePilz commented Aug 17, 2023

Copy link
Copy Markdown
Member Author

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.

@JorelAli

JorelAli commented Aug 17, 2023

Copy link
Copy Markdown
Member

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 👍

willkroboth
willkroboth requested changes Aug 17, 2023
View reviewed changes

@willkroboth willkroboth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

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

DerEchtePilz commented Aug 17, 2023

Copy link
Copy Markdown
Member Author

willkroboth
willkroboth requested changes Aug 24, 2023
View reviewed changes

@willkroboth willkroboth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread ...api-bukkit-kotlin-test/src/test/kotlin/dev/jorel/commandapi/test/dsltests/DelegationTests.kt
willkroboth
willkroboth approved these changes Aug 25, 2023
View reviewed changes

@willkroboth willkroboth left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

JorelAli
JorelAli reviewed Sep 22, 2023
View reviewed changes
Comment thread docssrc/src/kotlindsl.md Outdated
@DerEchtePilz DerEchtePilz merged commit 9620fd3 into dev/dev Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JorelAli JorelAli JorelAli approved these changes
@willkroboth willkroboth willkroboth approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DerEchtePilz @JorelAli @willkroboth