ottlinger commented on code in PR #708:
URL: https://github.com/apache/creadur-rat/pull/708#discussion_r3924507156
##########
apache-rat-core/src/main/java/org/apache/rat/commandline/Arg.java:
##########
@@ -977,8 +977,8 @@ private static <T> T[] getParsedOptionValues(final Option
selected, final Comman
try {
Class<? extends T> clazz = (Class<? extends T>) selected.getType();
String[] values = commandLine.getOptionValues(selected);
- T[] result = (T[]) Array.newInstance(clazz, values.length);
- for (int i = 0; i < values.length; i++) {
+ T[] result = (T[]) Array.newInstance(clazz, values == null ? 0 :
values.length);
+ for (int i = 0; i < result.length; i++) {
result[i] =
clazz.cast(selected.getConverter().apply(values[i]));
Review Comment:
There still remains a compiler warning that array access `values[i]` may
produce a NPE
##########
apache-rat-core/src/main/java/org/apache/rat/ui/UIOptionCollection.java:
##########
@@ -209,59 +249,51 @@ protected Builder(final BiFunction<UIOptionCollection<T>,
Option, T> mapper) {
}
/**
- * Build the UIOptionCollection.
- * @return the UIOptionCollection.
- */
- public UIOptionCollection<T> build() {
- return new UIOptionCollection<>(this);
- }
-
- /**
- * Returns this cast to {@code <S>} class.
- * @return this as {@code <S>} class.
+ * Returns this cast to {@code <B>} class.
+ * @return this as {@code <B>} class.
*/
- protected final S self() {
- return (S) this;
+ protected final B self() {
+ return (B) this;
}
/**
- * Add a UI option to the collection.
- * @param uiOption the UI Option to add.
+ * Add an Option to the collection as a UIOption.
+ * @param option the Option to add.
* @return this
*/
- public S uiOption(final Option uiOption) {
- uiOptions.add(uiOption);
+ public B uiOption(final Option option) {
+ uiOptions.add(option);
return self();
}
/**
- * Add a UI options to the collection.
- * @param uiOption the UIOptions ({@code <T>} objects) to add.
+ * Add multiple Option instances to the collection as UIOptions.
+ * @param options the Option instances to add.
* @return this
*/
- public S uiOptions(final Option... uiOption) {
- uiOptions.addAll(Arrays.asList(uiOption));
+ public B uiOptions(final Option... options) {
Review Comment:
The IDE marks this method as never used? Will it be used in upcoming PRs?
##########
apache-rat-core/src/main/java/org/apache/rat/ui/UIOption.java:
##########
@@ -240,6 +267,108 @@ public final String keyValue() {
* @return the deprecated string if the option is deprecated, or an empty
string otherwise.
*/
public final String getDeprecated() {
- return option.isDeprecated() ?
cleanup(StringUtils.defaultIfEmpty(option.getDeprecated().toString(),
StringUtils.EMPTY)) : StringUtils.EMPTY;
+ return option.isDeprecated() ?
cleanup(StringUtils.defaultIfEmpty(option.getDeprecated().toString(),
StringUtils.EMPTY)) : StringUtils.EMPTY;
+ }
+
+ /**
+ * The abstract UIOption Builder.
+ * @param <T> the concrete type of the UIOption this builder produces.
+ * @param <B> the concrete type of this builder.
+ */
+ public abstract static class Builder<T extends UIOption<T>, B extends
Builder<T, B>> {
+ /**
+ * The collection to add the new UI Option to.
+ */
+ private UIOptionCollection<T> optionCollection;
+ /**
+ * THe base option that is being mapped.
+ */
+ private Option option;
+ /**
+ * The UI name of this option.
+ */
+ private CasedString name = CasedString.NULL;
+ /**
+ * Constructor.
+ */
+ protected Builder() {
+ }
+
+ /**
+ * Returns a function to convert an Option to a CasedString that is
the native name for the option.
+ * @return a function to convert an Option to a CasedString that is
the native name for the option.
+ */
+ protected abstract Function<Option, CasedString> getNameFactory();
+
+ /**
+ * Gets the option.
+ * @return the option or {@code null} if the option is not set.
+ */
+ protected final Option option() {
+ return option;
+ }
+
+ /**
+ * Gets the UIOptionCollection that the options will be added to.
+ * @return UIOptionCollection that the options will be added to.
+ */
+ protected final UIOptionCollection<T> optionCollection() {
+ return optionCollection;
+ }
+
+ /**
+ * Returns this builder cast to the builder type.
+ * Useful for implementing fluent builders.
+ * @return this builder.
+ */
+ protected final B self() {
+ return (B) this;
+ }
+
+ /**
+ * Sets the UIOptionCollection that the UIOptions will be added to.
+ * @param optionCollection the UIOptionCollection that the UIOptions
will be added to.
+ * @return this
+ */
+ public B optionCollection(final UIOptionCollection<T>
optionCollection) {
+ this.optionCollection = optionCollection;
+ return self();
+ }
+
+ /**
+ * Sets the Option that the UIOption will be generated from.
+ * @param option to build the UIOption from.
+ * @return this
+ */
+ public B option(final Option option) {
+ Objects.requireNonNull(option, "Option may not be null");
+ this.option = option;
+ this.name = getNameFactory().apply(option);
+ if (this.name == null || this.name.isNull()) {
+ throw new IllegalArgumentException("name for " + option + "
may not be null or contain a null value");
+ }
+ return self();
+ }
+
+ /**
+ * Executes the final build.
+ * @return An instance of the UIOption.
+ * @throws IllegalArgumentException if values are not set correctly.
+ */
+ protected abstract T doBuild() throws IllegalArgumentException;
+
+ /**
+ * Builds the UIOption.
+ * @return the UIOption.
+ * @throws IllegalArgumentException if values are not set correctly.
+ */
+ public final T build() throws IllegalArgumentException {
+ Objects.requireNonNull(optionCollection, "OptionCollection may not
be null");
+ Objects.requireNonNull(option, "Option may not be null");
+ if (name == null || name.isNull()) {
+ throw new IllegalArgumentException("name may not be null or
contain a null value");
Review Comment:
is it really "may not be null" or should it be "must not be null" here?
##########
apache-rat-core/src/main/java/org/apache/rat/OptionCollectionParser.java:
##########
@@ -44,15 +46,21 @@
/**
* Uses the AbstractOptionCollection to parse the command line options.
* Contains utility methods to ReportConfiguration from the options and an
array of arguments.
+ *
+ * @param <T> The UIOption type that this parser is handeling.
*/
@SuppressFBWarnings("EI_EXPOSE_REP2")
-public final class OptionCollectionParser {
+public final class OptionCollectionParser<T extends UIOption<T>> {
Review Comment:
Inner class "OptionComparator" is never used? Should it be removed?
##########
apache-rat-core/src/main/java/org/apache/rat/utils/CasedString.java:
##########
@@ -45,6 +45,9 @@ public final class CasedString {
Arrays.stream(strings).map(s -> s == null ? "" : s).forEach(token ->
sb.append(WordUtils.capitalize(token.toLowerCase(Locale.ROOT))));
return sb.toString();
};
+ /** A null cased string */
+ // must follow CAMEL_JOINER def.
Review Comment:
Should we make this a real Javadoc reference?
##########
apache-rat-core/src/main/java/org/apache/rat/OptionCollection.java:
##########
@@ -188,25 +189,38 @@ public static synchronized ReportConfiguration
parseCommands(final File workingD
* @see #parseCommands(File, String[], Consumer, boolean)
*/
public static ReportConfiguration createConfiguration(final
ArgumentContext argumentContext) {
- argumentContext.processArgs(CLIOptionCollection.INSTANCE);
- final ReportConfiguration configuration =
argumentContext.getConfiguration();
- final CommandLine commandLine = argumentContext.getCommandLine();
- Optional<Option> dirOpt =
CLIOptionCollection.INSTANCE.getSelected(Arg.DIR);
- if (dirOpt.isPresent()) {
- try {
-
configuration.addSource(getReportable(commandLine.getParsedOptionValue(
- dirOpt.get()), configuration));
- } catch (ParseException e) {
- throw new ConfigurationException("Unable to set parse " +
dirOpt.get(), e);
+ try {
+ argumentContext.processArgs(baseOptionCollection);
+ final ReportConfiguration configuration =
argumentContext.getConfiguration();
+ final CommandLine commandLine = argumentContext.getCommandLine();
+ Optional<Option> dirOpt =
baseOptionCollection.getSelected(Arg.DIR);
Review Comment:
@Claudenw
The IDE issues a warning here:
`"Unchecked assignment: 'java.util.Optional' to 'Optional<Option>'. Reason
'baseOptionCollection' has raw type, so result of getSelected is erased
`
Is that an issue due to the new type hierarchy here?
##########
apache-rat-core/src/main/java/org/apache/rat/OptionCollectionParser.java:
##########
@@ -44,15 +46,21 @@
/**
* Uses the AbstractOptionCollection to parse the command line options.
* Contains utility methods to ReportConfiguration from the options and an
array of arguments.
+ *
+ * @param <T> The UIOption type that this parser is handeling.
*/
@SuppressFBWarnings("EI_EXPOSE_REP2")
-public final class OptionCollectionParser {
+public final class OptionCollectionParser<T extends UIOption<T>> {
Review Comment:
The field is instantiated, but never used at line 81.
##########
apache-rat-core/src/main/java/org/apache/rat/commandline/ArgumentContext.java:
##########
@@ -46,28 +50,51 @@ public final class ArgumentContext {
* Creates a context with the specified configuration.
* @param workingDirectory the directory from which relative file names
will be resolved.
* @param configuration The configuration that is being built.
- * @param commandLine The command line that is building the configuration.
+ * @param opts the Options for the command line.
+ * @param args the arguments for the options.
+ * @throws ParseException if the options can not parse the arguments.
*/
- public ArgumentContext(final File workingDirectory, final
ReportConfiguration configuration, final CommandLine commandLine) {
+ public ArgumentContext(final File workingDirectory, final
ReportConfiguration configuration, final Options opts, final String[] args)
+ throws ParseException {
this.workingDirectory = DocumentName.builder(workingDirectory).build();
- this.commandLine = commandLine;
+ this.commandLine =
OptionCollectionParser.parseCommandLine(clearSelected(opts), args);
this.configuration = configuration;
}
Review Comment:
That is already done, right or did you intend some more refactorings here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
