8000 Add Kotlin DSL support for delegations by DerEchtePilz · Pull Request #482 · CommandAPI/CommandAPI · GitHub
[go: up one dir, main page]
Skip to content

Add Kotlin DSL support for delegations #482

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Sep 23, 2023
Merged

Add Kotlin DSL support for delegations #482

merged 17 commits into from
Sep 23, 2023

Conversation

DerEchtePilz
Copy link
Member
@DerEchtePilz DerEchtePilz commented Aug 17, 2023 •
edited
Loading

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
Copy link
Member
  • 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?

@DerEchtePilz
Copy link
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
Copy link
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
Copy link
Collaborator
@willkroboth willkroboth left a comment

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.

@DerEchtePilz
Copy link
Member Author

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.

willkroboth
willkroboth requested changes Aug 24, 2023
View reviewed changes
Copy link
Collaborator
@willkroboth willkroboth left a comment

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.

willkroboth
willkroboth approved these changes Aug 25, 2023
View reviewed changes
Copy link
Collaborator
@willkroboth willkroboth left a comment •
edited
Loading

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