Rename Adventure arguments #474

DerEchtePilz · 2023-07-24T20:57:13Z

This essentially makes these changes to make arguments that deal with the same Minecraft argument type have identical names:

Spigot-related argument changes:

- dev.jorel.commandapi.arguments.ChatArgument
- dev.jorel.commandapi.arguments.ChatColorArgument
- dev.jorel.commandapi.arguments.ChatComponentArgument

+ dev.jorel.commandapi.arguments.spigot.ChatArgument
+ dev.jorel.commandapi.arguments.spigot.ChatColorArgument
+ dev.jorel.commandapi.arguments.spigot.ChatComponentArgument

Adventure-related argument changes:

- dev.jorel.commandapi.arguments.AdventureChatArgument
- dev.jorel.commandapi.arguments.AdventureChatColorArgument
- dev.jorel.commandapi.arguments.AdventureChatComponentArgument

+ dev.jorel.commandapi.arguments.adventure.ChatArgument
+ dev.jorel.commandapi.arguments.adventure.ChatColorArgument
+ dev.jorel.commandapi.arguments.adventure.ChatComponentArgument

TODO:

Replace instances of AdventureXArgument with XArgument
Update Kotlin DSL

DerEchtePilz

Yes, I did review my own PR :D
Basically, I just want to remind myself to actually make these changes when the time has come

README.md

DerEchtePilz · 2023-07-24T20:58:46Z

docssrc/src/upgrading.md

@@ -1,5 +1,35 @@
 # Upgrading guide

+## From 9.?.? to 10.0.0


Make this 9.?.? say the correct version when this is ready to be merged

JorelAli · 2023-07-25T00:35:14Z

The one mildly annoying thing with this PR is the developer experience. Developers now need to ensure that they import the "right one". There are no compile-time checks that will catch the case if a developer accidentally uses the wrong argument. (I'm only saying "mildly annoying", because the CommandAPI does have cases where it has classes which have the same name as other cases, such as NamespacedKey)

Not something worth rejecting the PR, just something to be aware of (of course, the fix for this is trivial on the developers point of view, and runtime errors will occur which should be detected during developer testing, so it's "easily identifiable"). We'll have to ensure that the documentation fully covers this to ensure that users don't mix them up (we should now have an intermediary "chat arguments" page, we can probably put a note about this there so developers know which package to import)

willkroboth · 2023-07-25T14:23:24Z

The one mildly annoying thing with this PR is the developer experience. Developers now need to ensure that they import the "right one". There are no compile-time checks that will catch the case if a developer accidentally uses the wrong argument. (I'm only saying "mildly annoying", because the CommandAPI does have cases where it has classes which have the same name as other cases, such as NamespacedKey)

Yeah, it is a little confusing. Similar things happened to me when I accidentally import java.awt.List instead of java.util.List.

Maybe this could be similar to those NamespacedKey examples. There could be a ChatArgument/ChatColorArgument... class for the Spigot case and ChatArgument.Adventure/ChatColorArgument.Adventure... for Adventure. Or, since Spigot doesn't have to be the 'default', there could be ChatArgument.Spigot/ChatColorArgument.Spigot... instead.

Going that way would pretty much be no change at all though. If it's not a big change, it's probably not going to solve the problem you are trying to solve.

Not something worth rejecting the PR, just something to be aware of (of course, the fix for this is trivial on the developers point of view, and runtime errors will occur which should be detected during developer testing, so it's "easily identifiable"). We'll have to ensure that the documentation fully covers this to ensure that users don't mix them up (we should now have an intermediary "chat arguments" page, we can probably put a note about this there so developers know which package to import)

As Skepter said, documentation is probably enough to solve any confusion from your current solution.

…XArgument

DerEchtePilz · 2023-09-24T20:28:27Z

Alright, I guess, this should be done.

Once again, the only thing I am really not sure about is documentation. In this case, the chatpreview.md page as that refers to different platform implementation several times. I personally think I found a good compromise between only naming the class name and mentioning the platform every time too.

So yeah, a code review would be appreciated for the whole PR :D

willkroboth

Just a few typos and small details I noticed. Otherwise, everything makes sense.

I'm still not sure what problem this is trying to solve. I've never used the chat arguments though, so I'm not sure how developers want to use them. One challenge I see with the setup here is providing support for Spigot and Adventure. Right now, if you want to use both ChatArguments in the same file, you need to use the fully qualified name (eg. dev.jorel.commandapi.arguments.spigot.ChatArgument) for at least one of them, which doesn't seem convenient. I don't know if anyone would want to do that, but this is currently happening in the ChatArgumentTests files.

I think placing the arguments as static classes like ChatArgument.Spigot and ChatArgument.Adventure fits better with existing Arguments (Eg. SoundArgument and SoundArgument.NamespacedKey both use the minecraft:resource_location Brigadier argument). I don't know though. The best solution probably depends on the problem that this is trying to solve.

If we think different packages is the best solution, this PR looks good to me 👍.

willkroboth · 2023-09-24T21:13:56Z

...tests/src/test/java/dev/jorel/commandapi/test/arguments/ArgumentAdventureChatColorTests.java

- * Tests for the {@link AdventureChatColorArgument}
+ * Tests for the {@link ChatColorArgument}
 */
 class ArgumentAdventureChatColorTests extends TestBase {


I noticed that the ArgumentAdventureChatColorTests are separate from the ArgumentChatColorTests. On the other hand, the ArgumentChatComponentTests and ArgumentChatTests combine the Spigot and Paper tests. I'm not sure which way is best, but I think it would make sense to be consistent.

Combining the tests might make sense given that the Arguments have the same names now. On the other hand, having the tests separate is good because you don't need to use a fully qualified name to differentiate one of the arguments.

willkroboth · 2023-09-24T21:27:24Z

...pi-documentation-code/src/main/kotlin/dev/jorel/commandapi/examples/kotlin/SpigotExamples.kt

+
+/* ANCHOR: argumentChatSpigot3 */
+CommandAPICommand("pbroadcast")
+    .withArguments(dev.jorel.commandapi.arguments.adventure.ChatArgument("message"))


Looks like this is using wrong class. This is supposed to be a Spigot example.

Suggested change

.withArguments(dev.jorel.commandapi.arguments.adventure.ChatArgument("message"))

.withArguments(ChatArgument("message"))

...dapi-documentation-code/src/main/java/dev/jorel/commandapi/examples/java/SpigotExamples.java

docssrc/src/chatpreview.md

DerEchtePilz · 2023-09-25T14:53:05Z

Just a few typos and small details I noticed. Otherwise, everything makes sense.

I'm still not sure what problem this is trying to solve. I've never used the chat arguments though, so I'm not sure how developers want to use them. One challenge I see with the setup here is providing support for Spigot and Adventure. Right now, if you want to use both ChatArguments in the same file, you need to use the fully qualified name (eg. dev.jorel.commandapi.arguments.spigot.ChatArgument) for at least one of them, which doesn't seem convenient. I don't know if anyone would want to do that, but this is currently happening in the ChatArgumentTests files.

I think placing the arguments as static classes like ChatArgument.Spigot and ChatArgument.Adventure fits better with existing Arguments (Eg. SoundArgument and SoundArgument.NamespacedKey both use the minecraft:resource_location Brigadier argument). I don't know though. The best solution probably depends on the problem that this is trying to solve.

If we think different packages is the best solution, this PR looks good to me 👍.

First, thanks for the review!
I am not really solving a problem with this since the arguments work fine the way they currently are, I just thought it would be nice if arguments that provide essentially the same functionality had the same name. My initial discord message
Of course, when I originally had the idea I didn't think about the case where someone would like to use both arguments in the same class (why would someone even want to do this, if you got Adventure, use Adventure).
Given that 10.0.0 isn't quite releasing any time soon, we still can discuss this package vs. inner class thing, I personally would continue to use this but using inner class would be fine as well.

…w.md

DerEchtePilz · 2024-01-19T15:11:25Z

Superseded by #517

DerEchtePilz added 2 commits July 24, 2023 22:14

Initial implementation of renaming Adventure arguments

feef8d6

Fix ChatComponentArgument import in BetterBossBarsCommand.java

849cd5e

DerEchtePilz commented Jul 24, 2023

View reviewed changes

DerEchtePilz marked this pull request as draft July 24, 2023 21:01

DerEchtePilz added the for annotations This feature will be worked on for the annotations update label Jul 24, 2023

DerEchtePilz changed the base branch from dev/dev to dev/10.0.0 August 8, 2023 15:42

DerEchtePilz added 8 commits August 17, 2023 12:31

Merge 'dev/dev' into 'dev/rename-adventure-arguments'

76b8922

Merge 'dev/10.0.0' into dev/rename-adventure-arguments

873db30

Merge branch 'dev/10.0.0' into dev/rename-adventure-arguments

18b5aea

Remove package identifier for platform-specific arguments

04c4162

Move platform-specific argument examples into their own class

805a3c1

Update Kotlin DSL methods for platform-specific arguments

d5bdcb5

Add Kotlin DSL package info in argument_chat_adventure.md

abc9cbb

Update arguments.md and chatpreview.md to no longer mention Adventure…

3a0ef5a

…XArgument

DerEchtePilz marked this pull request as ready for review September 24, 2023 20:12

DerEchtePilz added 2 commits September 24, 2023 22:20

Update upgrading.md to now also mention Kotlin DSL changes

0708bf4

Add separation line in upgrading.md

1df30d9

DerEchtePilz requested review from JorelAli and willkroboth September 24, 2023 20:28

willkroboth requested changes Sep 25, 2023

View reviewed changes

DerEchtePilz added 5 commits September 25, 2023 16:56

Remove left-over package identifier. Add missing spaces in chatprevie…

c9cad88

…w.md

Merge 'dev/10.0.0' into dev/rename-adventure-arguments

fd891c8

Merge 'dev/10.0.0' into dev/rename-adventure-arguments

94ed94c

Merge branch 'dev/10.0.0' into dev/rename-adventure-arguments

627baec

Merge branch 'dev/dev' into 'dev/rename-adventure-arguments'

5306135

DerEchtePilz marked this pull request as draft January 16, 2024 11:13

DerEchtePilz mentioned this pull request Jan 19, 2024

Paper CommandAPI #517

Merged

27 tasks

DerEchtePilz closed this Jan 19, 2024

	.withArguments(dev.jorel.commandapi.arguments.adventure.ChatArgument("message"))
	.withArguments(ChatArgument("message"))

Uh oh!

Rename Adventure arguments #474

Rename Adventure arguments #474

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DerEchtePilz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DerEchtePilz Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

JorelAli commented Jul 25, 2023

Uh oh!

willkroboth commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DerEchtePilz commented Sep 24, 2023

Uh oh!

willkroboth left a comment

Choose a reason for hiding this comment

Uh oh!

willkroboth Sep 24, 2023

Choose a reason for hiding this comment

Uh oh!

willkroboth Sep 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DerEchtePilz commented Sep 25, 2023

Uh oh!

DerEchtePilz commented Jan 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DerEchtePilz commented Jul 24, 2023 •

edited

Loading

willkroboth commented Jul 25, 2023 •

edited

Loading