Refactor UIOption - #708
Conversation
4bad3a9 to
b3c812a
Compare
…tions to provide cleaner implementations for multi threaded use as well as a better division of responsibility. - Implemented Builders for UIOption and UIOption derived classes. - Adjusted UIOPtionCollection to utilize UIOption.Builder - Simplified ArgumentContext construction by creating the CommandLine instance within the context. - Adapted command line parsing to accept UIOptionCollection as the source of arguments. - added isNull() to CasedString to indicate when the string will be null. - Updated tools to utilize new UIOption builders and collections.
1bd322e to
0173425
Compare
|
@ottlinger I like your changes. If you are happy approve and merge. |
| 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])); |
There was a problem hiding this comment.
There still remains a compiler warning that array access values[i] may produce a NPE
| 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"); |
There was a problem hiding this comment.
is it really "may not be null" or should it be "must not be null" here?
| */ | ||
| public S uiOptions(final Option... uiOption) { | ||
| uiOptions.addAll(Arrays.asList(uiOption)); | ||
| public B uiOptions(final Option... options) { |
There was a problem hiding this comment.
The IDE marks this method as never used? Will it be used in upcoming PRs?
There was a problem hiding this comment.
I believe so. When building a UI there are options that are specific to the UI (like the Maven option that excluded common patterns) that the UI will add. I think there are cases where there will be more than one.
| return sb.toString(); | ||
| }; | ||
| /** A null cased string */ | ||
| // must follow CAMEL_JOINER def. |
There was a problem hiding this comment.
Should we make this a real Javadoc reference?
| argumentContext.processArgs(baseOptionCollection); | ||
| final ReportConfiguration configuration = argumentContext.getConfiguration(); | ||
| final CommandLine commandLine = argumentContext.getCommandLine(); | ||
| Optional<Option> dirOpt = baseOptionCollection.getSelected(Arg.DIR); |
There was a problem hiding this 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?
There was a problem hiding this comment.
Fixed but will probalby change when the UI builds are extracted.
| */ | ||
| @SuppressFBWarnings("EI_EXPOSE_REP2") | ||
| public final class OptionCollectionParser { | ||
| public final class OptionCollectionParser<T extends UIOption<T>> { |
There was a problem hiding this comment.
Inner class "OptionComparator" is never used? Should it be removed?
There was a problem hiding this comment.
The field is instantiated, but never used at line 81.
|
|
@ottlinger I think I addressed all the issues. Please let me know if there are any outstanding. |
2 participants
Refactor UIOption, AntOption, and MavenOption along with their collections to provide cleaner implementations for multi threaded use as well as a better division of responsibility.