Skip to content
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.

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

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

Closed
CarmJos opened this issue Nov 26, 2023 · 28 comments · Fixed by #309 or #320
Closed

Comments

@CarmJos
Copy link
Contributor

CarmJos commented Nov 26, 2023

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.

@CarmJos CarmJos changed the title Considering use @Nullable to to mark a Optinal argument instead of using Optinal<?>. Considering support @Nullable to to mark a Optinal argument instead of using Optinal<?>. Nov 26, 2023
@CarmJos CarmJos changed the title Considering support @Nullable to to mark a Optinal argument instead of using Optinal<?>. Considering support @Nullable to to mark a Optinal argument instead of using Optinal<?>. Nov 26, 2023
@CarmJos CarmJos changed the title Considering support @Nullable to to mark a Optinal argument instead of using Optinal<?>. Considering support @Nullable to to mark a Optional argument instead of using Optinal<?>. Nov 26, 2023
@CarmJos CarmJos changed the title Considering support @Nullable to to mark a Optional argument instead of using Optinal<?>. Considering support @Nullable to to mark a Optional argument instead of using Optional<?>. Nov 26, 2023
@CarmJos CarmJos changed the title Considering support @Nullable to to mark a Optional argument instead of using Optional<?>. Considering support @Nullable to mark a Optional argument instead of using Optional<?>. Nov 26, 2023
@Rollczi
Copy link
Owner

Rollczi commented Nov 26, 2023

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

@CarmJos
Copy link
Contributor Author

CarmJos commented Nov 26, 2023

@CarmJos
Copy link
Contributor Author

CarmJos commented Nov 26, 2023

Or just provide a "@NullableArg"?

@Rollczi
Copy link
Owner

Rollczi commented Nov 26, 2023

Or just provide a "@NullableArg"?

I enjoy it, I will implement this feature.

@Rollczi
Copy link
Owner

Rollczi commented Nov 27, 2023

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
Copy link
Owner

Rollczi commented Nov 27, 2023

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

@CarmJos
Copy link
Contributor Author

CarmJos commented Nov 27, 2023

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

@CarmJos
Copy link
Contributor Author

CarmJos commented Nov 27, 2023

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
Copy link
Owner

Rollczi commented Nov 27, 2023

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
Copy link
Contributor Author

CarmJos commented Nov 27, 2023

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
Copy link
Contributor Author

CarmJos commented Nov 28, 2023

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
Copy link
Owner

Rollczi commented Nov 29, 2023

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
Copy link
Contributor Author

CarmJos commented Nov 29, 2023

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
Copy link
Owner

Rollczi commented Nov 29, 2023

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
Copy link
Contributor

rchomczyk commented Nov 29, 2023

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

@CarmJos
Copy link
Contributor Author

CarmJos commented Nov 30, 2023

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 CarmJos changed the title Considering support @Nullable to mark a Optional argument instead of using Optional<?>. Considering support @OptionalArg to mark a Optional argument instead of using Optional<?>. Nov 30, 2023
@CarmJos
Copy link
Contributor Author

CarmJos commented Nov 30, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

CarmJos commented Nov 30, 2023

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
Copy link
Owner

Rollczi commented Dec 1, 2023

You are actually right, Option is not smart.

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

optional keyword

  • it better represents the concept of optional arguments.

nullable keyword

  • this indicates that the argument is nullable.

@KeywordArg

  • This annotation is smaller than the use case of @Arg with attributes.

@Arg(keyword = true)

  • There are no new annotations. It is more consistent with the rest of the API
  • The feature is more closely related to the annotation. (Similarly to the @RequestParm)

@Rollczi
Copy link
Owner

Rollczi commented Dec 1, 2023

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
Copy link
Owner

Rollczi commented Dec 1, 2023

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
Copy link
Contributor Author

CarmJos commented Dec 2, 2023

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
Copy link
Owner

Rollczi commented Dec 2, 2023

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
Copy link
Contributor Author

CarmJos commented Dec 2, 2023

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
Copy link
Contributor

private String generateExecutor(CommandExecutor<SENDER> executor) {
return executor.getArguments().stream()
.map(argument -> String.format(this.isOptional(argument) ? format.optionalArgumentFormat() : format.argumentFormat(), argument.getName()))
.collect(Collectors.joining(SEPARATOR));
}

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

@Rollczi
Copy link
Owner

Rollczi commented Dec 2, 2023

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?

Rollczi added a commit that referenced this issue Dec 2, 2023
* Add nullable arguments

* Replace nullable to optional
@github-project-automation github-project-automation bot moved this from Todo to Done in LiteCommands Dec 2, 2023
@Rollczi Rollczi reopened this Dec 2, 2023
@github-project-automation github-project-automation bot moved this from Done to In Progress in LiteCommands Dec 2, 2023
@CarmJos
Copy link
Contributor Author

CarmJos commented Dec 3, 2023

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.

Rollczi added a commit that referenced this issue Dec 4, 2023
* Add nullable arguments

* Replace nullable to optional

* Add OptionalArg annotation

* Update programmatic API

* Update nullable argument api for programmatic API.
@github-project-automation github-project-automation bot moved this from In Progress to Done in LiteCommands Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
4 participants