Rollczi / LiteCommands

☄️ LiteCommands - Command framework for Velocity, Bukkit, Paper, BungeeCord, Minestom, Sponge, Fabric, JDA and future implementations.
https://docs.rollczi.dev/
Apache License 2.0
147 stars 21 forks source link

Considering support `@OptionalArg` to mark a Optional argument instead of using `Optional<?>`. #305

Closed CarmJos closed 11 months ago

CarmJos commented 1 year ago

Using org.jetbrains.annotations.Nullable instead of using Optinal<?> in function's arguments will avoid many issues while also complying with general Java development specifications.

Otherwise, @SuppressWarnings("OptionalUsedAsFieldOrParameterType") is always needed, and will still influence code factors' score.

Rollczi commented 1 year ago

@CarmJos, org.jetbrains.annotations.Nullable is annotated with RetentionPolicy.CLASS image

CarmJos commented 1 year ago

How about @checkerframework.checker.nullness.qual.Nullable ?

CarmJos commented 1 year ago

Or just provide a "@NullableArg"?

Rollczi commented 1 year ago

Or just provide a "@NullableArg"?

I enjoy it, I will implement this feature.

Rollczi commented 1 year ago

hmmm, image @NullableArg seems to be a bit longer than the other arguments. I think your first concept is better, but also I don't want to take the Nullable (this will interfere with the use of jetbrains annotations)

Rollczi commented 1 year ago

There is also an option to add attribute @Arg(nullable = true)

CarmJos commented 1 year ago

Well, that's great enough. Thanks a lot!

CarmJos commented 1 year ago

hmmm, image @NullableArg seems to be a bit longer than the other arguments. I think your first concept is better, but also I don't want to take the Nullable (this will interfere with the use of jetbrains annotations)

In my opinion,This annotation may longer than others but it can describe the argument more clearly. Also, We entered the single @NullableArg annotation will better than add the nullable = true in @Arg annotation due to shorter length and ide support.

Rollczi commented 1 year ago

hmmm, image @NullableArg seems to be a bit longer than the other arguments. I think your first concept is better, but also I don't want to take the Nullable (this will interfere with the use of jetbrains annotations)

In my opinion,This annotation may longer than others but it can describe the argument more clearly. Also, We entered the single @NullableArg annotation will better than add the nullable = true in @Arg annotation due to shorter length and ide support.

I asked LiteCommands community about this feature on discord Also, you can vote for it -> pull

I got another way (from @P3ridot) that would be more consistent with the current API: @Arg(optional = true)

CarmJos commented 1 year ago

hmmm, image @NullableArg seems to be a bit longer than the other arguments. I think your first concept is better, but also I don't want to take the Nullable (this will interfere with the use of jetbrains annotations)

In my opinion,This annotation may longer than others but it can describe the argument more clearly. Also, We entered the single @NullableArg annotation will better than add the nullable = true in @Arg annotation due to shorter length and ide support.

I asked LiteCommands community about this feature on discord Also, you can vote for it -> pull

I got another way (from @P3ridot) that would be more consistent with the current API: @Arg(optional = true)

Followed by this, Maybe we can support a new annotation called '@OptionalArg'.

CarmJos commented 12 months ago

Another main reason why I personally do not recommend adding new parameters to @Arg is that when I also need to set a name for the optional argument, the entire annotation will become very long, which looks like this:

@Arg(value = "target",nullable = true) Player player

If using the @OptionalArg, it will look like this:

@OptionalArg("target") Player player
Rollczi commented 12 months ago

Another main reason why I personally do not recommend adding new parameters to @Arg is that when I also need to set a name for the optional argument, the entire annotation will become very long, which looks like this:

@Arg(value = "target",nullable = true) Player player

Yeah, you are right. Poll don't clearly show a better solution to do this, but I think your @NullableArg seems to be the best way in this situation. image

So, I will implement @NullableArg. Thanks for your help!

CarmJos commented 12 months ago

Another main reason why I personally do not recommend adding new parameters to @Arg is that when I also need to set a name for the optional argument, the entire annotation will become very long, which looks like this:

@Arg(value = "target",nullable = true) Player player

Yeah, you are right. Poll don't clearly show a better solution to do this, but I think your @NullableArg seems to be the best way in this situation. image

So, I will implement @NullableArg. Thanks for your help!

Well, after deeplyconsideration, I think use the @OptionalArg to name this annotation may be better.

Rollczi commented 12 months ago

Well, after deeplyconsideration, I think use the @OptionalArg to name this annotation may be better.

It is also a good solution. This gives me some food for thought... Hm... what do you think about short alternative @OptionArg?

rchomczyk commented 12 months ago

From my view the @OptionalArg is better name, as the option phrase could be misleading in that context.

CarmJos commented 12 months ago

Well, after deeplyconsideration, I think use the @OptionalArg to name this annotation may be better.

It is also a good solution. This gives me some food for thought... Hm... what do you think about short alternative @OptionArg?

@OptionArg is okay, but may cause some misunderstandings (e.g. --option). As a result, in my opinion, the @OptionalArg is better, as the same name and behavior with Optional<> in arguments.

CarmJos commented 12 months ago

I have viewed another issues (#310 ) contained a @Flag annotation for --option xyz123 as well as -o xyz123 function. I think the @OptionArg should be reserved or be avoided for this function.

huanmeng-qwq commented 12 months ago

I think there is a certain functional overlap between @OptionArg and @Flag. If I were to choose between the two, I would choose @Flag. If you keep both, it would give developers an additional choice

CarmJos commented 12 months ago

I think there is a certain functional overlap between @OptionArg and @Flag. If I were to choose between the two, I would choose @Flag. If you keep both, it would give developers an additional choice

This is just an example. I wish that @OptionalArg will not be confused with @Flag (@OptionArg has a similar meaning), so I give a special example.

Rollczi commented 12 months ago

You are actually right, Option is not smart.

After much discussion and research, I have decided to write arguments for:

optional keyword

nullable keyword

@KeywordArg

@Arg(keyword = true)

Rollczi commented 12 months ago

Another main reason why I personally do not recommend adding new parameters to @Arg is that when I also need to set a > name for the optional argument, the entire annotation will become very long, which looks like this:

@Arg(value = "target", nullable = true) Player player

If you add flag -parameters to java compilation then you can use it to define the name of argument based on the parameter name, which should shorten the code to:

@Arg(optional = true) Player target
Rollczi commented 12 months ago

optional is great. Also, I don't want to add new annotations to the API to make it more complicated. This is a challenging decision because all solutions have pros and cons. I think that's the best solution for the moment, looking at the widely used Spring Boot API.

CarmJos commented 12 months ago

Another main reason why I personally do not recommend adding new parameters to @Arg is that when I also need to set a > name for the optional argument, the entire annotation will become very long, which looks like this:

@Arg(value = "target", nullable = true) Player player

If you add flag -parameters to java compilation then you can use it to define the name of argument based on the parameter name, which should shorten the code to:

@Arg(optional = true) Player target

In this example, I use an English arg name. But in reality, the name is often filled in Chinese to prompt the user. Which means I must use this name, instead of filling it in the parameters of the method.

Rollczi commented 12 months ago

Another main reason why I personally do not recommend adding new parameters to @Arg is that when I also need to set a > name for the optional argument, the entire annotation will become very long, which looks like this:

@Arg(value = "target", nullable = true) Player player

If you add flag -parameters to java compilation then you can use it to define the name of argument based on the parameter name, which should shorten the code to:

@Arg(optional = true) Player target

In this example, I use an English arg name. But in reality, the name is often filled in Chinese to prompt the user. Which means I must use this name, instead of filling it in the parameters of the method.

Unfortunately, in some cases it will be annoying :/

CarmJos commented 12 months ago

Another main reason why I personally do not recommend adding new parameters to @arg is that when I also need to set a > name for the optional argument, the entire annotation will become very long, which looks like this:

@Arg(value = "target", nullable = true) Player player

If you add flag -parameters to java compilation then you can use it to define the name of argument based on the parameter name, which should shorten the code to:

@Arg(optional = true) Player target

In this example, I use an English arg name. But in reality, the name is often filled in Chinese to prompt the user. Which means I must use this name, instead of filling it in the parameters of the method.

Unfortunately, in some cases it will be annoying :/

Yeah, so please think about non-English developers, it will be very disastrous if they can only use @Arg(value = "目标玩家", nullable = true) form.

huanmeng-qwq commented 12 months ago

https://github.com/Rollczi/LiteCommands/blob/b7ecb1606ee933f973a8ca5d1fb6a354d115d621/litecommands-core/src/dev/rollczi/litecommands/schematic/SchematicGeneratorSimpleImpl.java#L62-L66

I think this part should be abstracted so that developers can customize it, including argument

Rollczi commented 12 months ago

Yeah, so please think about non-English developers, it will be very disastrous if they can only use @Arg(value = "目标玩家", nullable = true) form.

This is another argument for @OptionalArg. I always wait for the feedback in the issue before merge. I know the discussion is long, but it is very good for development 😁

I was thinking about implementing two ideas at once, but it probably won't be good for the API...

So we're left with @OptionalArg if no one has anything to add to the discussion?

CarmJos commented 11 months ago

Yeah, so please think about non-English developers, it will be very disastrous if they can only use @Arg(value = "目标玩家", nullable = true) form.

This is another argument for @OptionalArg. I always wait for the feedback in the issue before merge. I know the discussion is long, but it is very good for development 😁

I was thinking about implementing two ideas at once, but it probably won't be good for the API...

So we're left with @OptionalArg if no one has anything to add to the discussion?

It seems right, there's only @OptionalArg left.