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

[#1380] Fix for requiredOptionMarker displayed on ArgGroup #1505

Merged
merged 20 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package picocli;

import org.junit.Test;
import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Command;
import picocli.CommandLine.Option;

import java.io.ByteArrayOutputStream;
import java.io.PrintStream;

import static org.junit.Assert.assertEquals;

/**
* Testing class for exclusiveOptions
*/
class Issue1380ExclusiveOptions {
@Option(names = {"-s", "--silent"}, description = "Silent mode", required = false)
protected boolean silent;

@Option(names = {"-v", "--verbose"}, description = "Verbose mode", required = false)
protected boolean verbose;

@Option(names = {"-j", "--json"}, description = "JSON printing", required = false)
protected boolean json;
}

/**
* Testing class for creating a commandline with ArgGroup exclusive tree
*/
@Command(requiredOptionMarker = '*')
class TestingClassExclusiveTrue {

@ArgGroup(exclusive = true, multiplicity = "0..1")
protected Issue1380ExclusiveOptions exclusive;
}

/**
* Testing class for creating a commandline with ArgGroup exclusive false
*/
@Command(requiredOptionMarker = '*')
class TestingClassExclusiveFalse {

@ArgGroup(exclusive = false, multiplicity = "0..1")
protected Issue1380ExclusiveOptions exclusive;
}

/**
* JUnit testing class for issue#1380 // CS427 https://github.com/remkop/picocli/issues/1380
*/
public class Issue1380Test {

/**
* JUnit test class for issue#1380 with exclusive set to true // CS427 https://github.com/remkop/picocli/issues/1380
*/
@Test
public void testingWithExclusiveTrue() {
ByteArrayOutputStream tempOut = new ByteArrayOutputStream();
PrintStream printStream = new PrintStream(tempOut);
new CommandLine(new TestingClassExclusiveTrue()).usage(printStream);

String returnedText = tempOut.toString();
String expectedText =
"Usage: <main class> [-s | -v | -j]\n"+
" -j, --json JSON printing\n"+
" -s, --silent Silent mode\n"+
" -v, --verbose Verbose mode\n";

assertEquals(expectedText, returnedText);
}

/**
* JUnit test class for issue#1380 with exclusive set to false// CS427 https://github.com/remkop/picocli/issues/1380
*/
@Test
public void testingWithExclusiveFalse() {
ByteArrayOutputStream tempOut = new ByteArrayOutputStream();
PrintStream printStream = new PrintStream(tempOut);
new CommandLine(new TestingClassExclusiveFalse()).usage(printStream);

String returnedText = tempOut.toString();
String expectedText = "Usage: <main class> [[-s] [-v] [-j]]\n" +
" -j, --json JSON printing\n" +
" -s, --silent Silent mode\n" +
" -v, --verbose Verbose mode\n";

assertEquals(expectedText, returnedText);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package picocli;

import org.junit.Ignore;
import org.junit.Test;
import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Command;
import picocli.CommandLine.Option;

import java.io.ByteArrayOutputStream;
import java.io.PrintStream;

import static org.junit.Assert.assertEquals;

/**
* Testing class for creating a commandline with resourceBundle
*/
@Command(name = "TestingCommand", resourceBundle = ("picocli.Message"))
class TestingClass {

@ArgGroup(exclusive = true, multiplicity = "0..1")
protected TestingClass.ExclusiveOptions exclusive;

public TestingClass.ExclusiveOptions getExclusive() {
return this.exclusive;
}
private static class ExclusiveOptions {
@Option(names = {"-h", "--help"}, descriptionKey = "help.message", required = false)
protected boolean showHelp;

@Option(names = {"-j", "--json"}, descriptionKey = "json.message", required = false)
protected boolean isJson;

}
}

@Ignore
/**
* JUnit testing class for issue#1420 // CS427 https://github.com/remkop/picocli/issues/1420
*/
public class Issue1420Test {

/**
* JUnit test class for issue#1420 with resourceBundle // CS427 https://github.com/remkop/picocli/issues/1420
*/
@Test
public void testingWithResourceBundle1() {
ByteArrayOutputStream tempOut = new ByteArrayOutputStream();
PrintStream printStream = new PrintStream(tempOut);
new CommandLine(new TestingClass()).usage(printStream);

String returnedText = tempOut.toString();
String expectedText = "Usage: TestingCommand [[-h] | [-j]]\n" +
" -h, --help Show help options\n" +
" -j, --json Set json export-on\n";

assertEquals(expectedText, returnedText);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* Testing class for // CS427 https://github.com/remkop/picocli/issues/1420
*/
help.message=Show help options
json.message =Set json export-on

15 changes: 14 additions & 1 deletion src/main/java/picocli/CommandLine.java
Original file line number Diff line number Diff line change
Expand Up @@ -8528,6 +8528,7 @@ public abstract static class ArgSpec {

// parser fields
private boolean required;
private boolean originallyRequired;
remkop marked this conversation as resolved.
Show resolved Hide resolved
private final boolean interactive;
private final boolean echo;
private final String prompt;
Expand Down Expand Up @@ -8580,6 +8581,7 @@ private <T extends Builder<T>> ArgSpec(Builder<T> builder) {
annotatedElement = builder.annotatedElement;
defaultValue = NO_DEFAULT_VALUE.equals(builder.defaultValue) ? null : builder.defaultValue;
required = builder.required;
originallyRequired = builder.originallyRequired;
toString = builder.toString;
getter = builder.getter;
setter = builder.setter;
Expand Down Expand Up @@ -8636,6 +8638,13 @@ void applyInitialValue(Tracer tracer) {
}
}

/** Returns the original value of the option's required attribute, regardless of whether the option is used in an exclusive group or not.
Copy link
Owner

Choose a reason for hiding this comment

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

Let's document that this is the original value of the option's required value, regardless of whether the option is in an exclusive group or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Owner

Choose a reason for hiding this comment

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

How about this instead:

"Returns the original value of the option's required attribute, regardless of whether the option is used in an exclusive group or not."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

* @since 4.7.0
remkop marked this conversation as resolved.
Show resolved Hide resolved
* @see Option#required() */
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove this: there is no @Option.optionIsNotRequired or @Option.originallyRequired method on the @Option annotation. Maybe link to @see OptionSpec#required() ?

Also, since we are adding public API, let's add a @since tag.
This will be included in the 4.7.0 release, so let's add @since 4.7.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

public boolean originallyRequired(){
remkop marked this conversation as resolved.
Show resolved Hide resolved
return originallyRequired;
}

/** Returns whether this is a required option or positional parameter without a default value.
* If this argument is part of a {@linkplain ArgGroup group}, this method returns whether this argument is required <em>within the group</em> (so it is not necessarily a required argument for the command).
* @see Option#required() */
Expand Down Expand Up @@ -9187,6 +9196,7 @@ abstract static class Builder<T extends Builder<T>> {
private String[] description;
private String descriptionKey;
private boolean required;
private boolean originallyRequired;
private boolean interactive;
private boolean echo;
private String prompt;
Expand Down Expand Up @@ -9224,6 +9234,7 @@ abstract static class Builder<T extends Builder<T>> {
description = original.description;
descriptionKey = original.descriptionKey;
required = original.required;
originallyRequired = original.originallyRequired;
interactive = original.interactive;
echo = original.echo;
prompt = original.prompt;
Expand Down Expand Up @@ -10121,6 +10132,8 @@ public static class ArgGroupSpec implements IOrdered {
if (!arg.required()) {
modifiedArgs += sep + (arg.isOption() ? ((OptionSpec) arg).longestName() : (arg.paramLabel() + "[" + ((PositionalParamSpec) arg).index() + "]"));
sep = ",";
//Keep initial required as originallyRequired for Issue#1380 https://github.com/remkop/picocli/issues/1380
arg.originallyRequired = true;
arg.required = true;
}
}
Expand Down Expand Up @@ -16115,7 +16128,7 @@ public Text[][] render(OptionSpec option, IParamLabelRenderer paramLabelRenderer
String longOption = join(names, shortOptionCount, names.length - shortOptionCount, ", ");
Text longOptionText = createLongOptionText(option, paramLabelRenderer, scheme, longOption);

String requiredOption = option.required() ? requiredMarker : "";
String requiredOption = !option.originallyRequired() && option.required() ? requiredMarker : "";
return renderDescriptionLines(option, scheme, requiredOption, shortOption, longOptionText);
}

Expand Down