ottlinger commented on code in PR #637:
URL: https://github.com/apache/creadur-rat/pull/637#discussion_r3255570320
##########
apache-rat-core/src/main/java/org/apache/rat/commandline/Arg.java:
##########
@@ -806,82 +912,22 @@ public static void processLogLevel(final CommandLine
commandLine) {
* @param context the context in which to process the args.
* @throws ConfigurationException on error
*/
- public static void processArgs(final ArgumentContext context) throws
ConfigurationException {
+ public static void processArgs(final ArgumentContext context, final
UIOptionCollection<?> optionCollection) throws ConfigurationException {
Converters.FILE_CONVERTER.setWorkingDirectory(context.getWorkingDirectory());
- processOutputArgs(context);
- processEditArgs(context);
- processInputArgs(context);
- processConfigurationArgs(context);
+ processOutputArgs(context, optionCollection);
+ processEditArgs(context, optionCollection);
+ processInputArgs(context, optionCollection);
+ processConfigurationArgs(context, optionCollection);
}
/**
* Process the arguments that can be processed together.
*
* @param context the context in which to process the args.
*/
- private static void processOutputArgs(final ArgumentContext context) {
- context.getConfiguration().setDryRun(DRY_RUN.isSelected());
-
- if (OUTPUT_FAMILIES.isSelected()) {
- try {
-
context.getConfiguration().listFamilies(context.getCommandLine().getParsedOptionValue(OUTPUT_FAMILIES.getSelected()));
- } catch (ParseException e) {
- context.logParseException(e, OUTPUT_FAMILIES.getSelected(),
Defaults.LIST_FAMILIES);
- }
- }
-
- if (OUTPUT_LICENSES.isSelected()) {
- try {
-
context.getConfiguration().listLicenses(context.getCommandLine().getParsedOptionValue(OUTPUT_LICENSES.getSelected()));
- } catch (ParseException e) {
- context.logParseException(e, OUTPUT_LICENSES.getSelected(),
Defaults.LIST_LICENSES);
- }
- }
-
- if (OUTPUT_ARCHIVE.isSelected()) {
- try {
-
context.getConfiguration().setArchiveProcessing(context.getCommandLine().getParsedOptionValue(OUTPUT_ARCHIVE.getSelected()));
- } catch (ParseException e) {
- context.logParseException(e, OUTPUT_ARCHIVE.getSelected(),
Defaults.ARCHIVE_PROCESSING);
- }
- }
-
- if (OUTPUT_STANDARD.isSelected()) {
- try {
-
context.getConfiguration().setStandardProcessing(context.getCommandLine().getParsedOptionValue(OUTPUT_STANDARD.getSelected()));
- } catch (ParseException e) {
- context.logParseException(e, OUTPUT_STANDARD.getSelected(),
Defaults.STANDARD_PROCESSING);
- }
- }
-
- if (OUTPUT_FILE.isSelected()) {
- try {
- File file =
context.getCommandLine().getParsedOptionValue(OUTPUT_FILE.getSelected());
- File parent = file.getParentFile();
- if (!parent.mkdirs() && !parent.isDirectory()) {
- DefaultLog.getInstance().error("Could not create report
parent directory " + file);
- }
- context.getConfiguration().setOut(file);
- } catch (ParseException e) {
- context.logParseException(e, OUTPUT_FILE.getSelected(),
"System.out");
- context.getConfiguration().setOut((IOSupplier<OutputStream>)
null);
- }
- }
-
- if (OUTPUT_STYLE.isSelected()) {
- String selected = OUTPUT_STYLE.getSelected().getKey(); // is not
null due to above isSelected()-call
- if ("x".equals(selected)) {
- // display deprecated message.
- context.getCommandLine().hasOption("x");
-
context.getConfiguration().setStyleSheet(StyleSheets.getStyleSheet("xml"));
- } else {
- String[] style =
context.getCommandLine().getOptionValues(OUTPUT_STYLE.getSelected());
- if (style.length != 1) {
- DefaultLog.getInstance().error("Please specify a single
stylesheet");
- throw new ConfigurationException("Please specify a single
stylesheet");
- }
-
context.getConfiguration().setStyleSheet(StyleSheets.getStyleSheet(style[0]));
- }
+ private static void processOutputArgs(final ArgumentContext context, final
UIOptionCollection<?> optionCollection) throws ConfigurationException {
+ for (Arg arg : new Arg[]{DRY_RUN, OUTPUT_FAMILIES, OUTPUT_LICENSES,
OUTPUT_ARCHIVE, OUTPUT_STANDARD, OUTPUT_FILE, OUTPUT_STYLE}) {
Review Comment:
Personally I'd prefer List.of() due to better readability than arrays here
....
##########
apache-rat-core/src/main/java/org/apache/rat/commandline/Arg.java:
##########
@@ -692,91 +875,21 @@ private static void processConfigurationArgs(final
ArgumentContext context) thro
* @param context the context to work in.
* @throws ConfigurationException if an exclude file can not be read.
*/
- private static void processInputArgs(final ArgumentContext context) throws
ConfigurationException {
- try {
- if (SOURCE.isSelected()) {
- File[] files =
SOURCE.getParsedOptionValues(context.getCommandLine());
- for (File f : files) {
- context.getConfiguration().addSource(f);
- }
- }
- // TODO when include/exclude processing is updated check calling
methods to ensure that all specified
- // directories are handled in the list of directories.
- if (EXCLUDE.isSelected()) {
- String[] excludes =
context.getCommandLine().getOptionValues(EXCLUDE.getSelected());
- if (excludes != null) {
-
context.getConfiguration().addExcludedPatterns(Arrays.asList(excludes));
- }
- }
- if (EXCLUDE_FILE.isSelected()) {
- File excludeFileName =
context.getCommandLine().getParsedOptionValue(EXCLUDE_FILE.getSelected());
- if (excludeFileName != null) {
-
context.getConfiguration().addExcludedPatterns(ExclusionUtils.asIterable(excludeFileName,
"#"));
- }
- }
- if (EXCLUDE_STD.isSelected()) {
- for (String s :
context.getCommandLine().getOptionValues(EXCLUDE_STD.getSelected())) {
-
context.getConfiguration().addExcludedCollection(StandardCollection.valueOf(s));
- }
- }
- if (EXCLUDE_PARSE_SCM.isSelected()) {
- StandardCollection[] collections =
EXCLUDE_PARSE_SCM.getParsedOptionValues(context.getCommandLine());
- final ReportConfiguration configuration =
context.getConfiguration();
- for (StandardCollection collection : collections) {
- if (collection == StandardCollection.ALL) {
-
Arrays.asList(StandardCollection.values()).forEach(configuration::addExcludedFileProcessor);
-
Arrays.asList(StandardCollection.values()).forEach(configuration::addExcludedCollection);
- } else {
- configuration.addExcludedFileProcessor(collection);
- configuration.addExcludedCollection(collection);
- }
- }
- }
- if (EXCLUDE_SIZE.isSelected()) {
- final int maxSize =
EXCLUDE_SIZE.getParsedOptionValue(context.getCommandLine());
- DocumentNameMatcher matcher = new
DocumentNameMatcher(String.format("File size < %s bytes", maxSize),
- (Predicate<DocumentName>) documentName -> {
- File f = new File(documentName.getName());
- return f.isFile() && f.length() < maxSize;
- });
- context.getConfiguration().addExcludedMatcher(matcher);
- }
- if (INCLUDE.isSelected()) {
- String[] includes =
context.getCommandLine().getOptionValues(INCLUDE.getSelected());
- if (includes != null) {
-
context.getConfiguration().addIncludedPatterns(Arrays.asList(includes));
- }
- }
- if (INCLUDE_FILE.isSelected()) {
- File includeFileName =
context.getCommandLine().getParsedOptionValue(INCLUDE_FILE.getSelected());
- if (includeFileName != null) {
-
context.getConfiguration().addIncludedPatterns(ExclusionUtils.asIterable(includeFileName,
"#"));
- }
- }
- if (INCLUDE_STD.isSelected()) {
- Option selected = INCLUDE_STD.getSelected();
- // display deprecation log if needed.
- if
(context.getCommandLine().hasOption("scan-hidden-directories")) {
-
context.getConfiguration().addIncludedCollection(StandardCollection.HIDDEN_DIR);
- } else {
- for (String s :
context.getCommandLine().getOptionValues(selected)) {
-
context.getConfiguration().addIncludedCollection(StandardCollection.valueOf(s));
- }
- }
- }
- } catch (Exception e) {
- throw ConfigurationException.from(e);
+ private static void processInputArgs(final ArgumentContext context, final
UIOptionCollection<?> optionCollection) throws ConfigurationException {
+ for (Arg arg : new Arg[]{SOURCE, EXCLUDE, EXCLUDE_FILE, EXCLUDE_STD,
EXCLUDE_PARSE_SCM, EXCLUDE_SIZE,
Review Comment:
Personally I'd prefer List.of() due to better readability than arrays here
....
##########
apache-rat-plugin/src/it/RAT-508/pom.xml:
##########
@@ -34,12 +34,8 @@
<verbose>true</verbose>
<consoleOutput>true</consoleOutput>
<ignoreErrors>false</ignoreErrors>
- <inputIncludes>
- <include>pom.xml</include>
- </inputIncludes>
- <inputExcludeStds>
- <exclude>ALL</exclude>
- </inputExcludeStds>
+ <inputIncludes>pom.xml</inputIncludes>
+ <inputExcludeStds>ALL</inputExcludeStds>
Review Comment:
Should we document this breaking change in the change-notes for 1.0.0
explicitly?
##########
apache-rat-tools/src/main/java/org/apache/rat/tools/AntDocumentation.java:
##########
@@ -79,13 +77,13 @@ public static void main(final String[] args) {
new AntDocumentation(outputDir).execute();
}
- private AntDocumentation(final File outputDir) {
+ /* Visible for testing */
Review Comment:
Class contains unused methods:
* writeTitle
* writePara
* writeList
Should they be removed or will they be needed later and are only yet unused?
##########
apache-rat-core/src/main/java/org/apache/rat/commandline/Arg.java:
##########
@@ -337,7 +420,17 @@ public enum Arg {
.desc("Reads <Expression> entries from a file. Entries
will be excluded from processing.")
.deprecated(DeprecatedAttributes.builder().setForRemoval(true).setSince("0.17")
.setDescription(StdMsgs.useMsg("--input-include-file")).get())
- .build())),
+ .build()),
+ (context, selected) -> {
+ try {
+ File includeFileName =
context.getCommandLine().getParsedOptionValue(selected);
+ if (includeFileName != null) {
+
context.getConfiguration().addIncludedPatterns(ExclusionUtils.asIterable(includeFileName,
"#"));
Review Comment:
Description says "entries will be excluded from processing", but the method
is "addIncludePatterns" - shouldn't it be "addExcludePatterns" then?
Most probably the description of the deprecated option is misleading ....
##########
apache-rat-core/src/main/java/org/apache/rat/commandline/Arg.java:
##########
@@ -475,51 +653,66 @@ public enum Arg {
.addOption(Option.builder().longOpt("output-standard").hasArg().argName("ProcessingType")
.desc("Specifies the level of detail in STANDARD file
reporting.")
.converter(s ->
ReportConfiguration.Processing.valueOf(s.toUpperCase()))
- .build())),
+ .build()),
+ (context, selected) -> {
+ try {
+
context.getConfiguration().setStandardProcessing(context.getCommandLine().getParsedOptionValue(selected));
+ } catch (ParseException e) {
+ context.logParseException(e, selected,
Defaults.STANDARD_PROCESSING);
+ }
+ }),
/**
* Provide license definition listing of registered licenses.
*/
HELP_LICENSES(new OptionGroup()
.addOption(Option.builder().longOpt("help-licenses") //
- .desc("Print information about registered
licenses.").build()));
+ .desc("Print information about registered
licenses.").build()),
+ Arg::doNotExecute
+ );
- /** The option group for the argument */
+ /**
+ * The option group for the argument
+ */
private final OptionGroup group;
+ /**
+ * The BiConsumer to apply the option to update the state of the
context.configuration.
+ */
+ private final BiConsumer<ArgumentContext, Option> process;
+
+
+ private static void doNotExecute(final ArgumentContext context, final
Option selected) {
+ throw new ImplementationException(String.format("'%s' should not be
executed directly", selected));
Review Comment:
Initially I wondered "... what to do instead" is missing in that message to
the user of RAT.
If I get this message, did I do something wrong or not?
In the case of "help-licenses" I used the option correctly ....
##########
apache-rat-core/src/test/java/org/apache/rat/ReporterOptionsProvider.java:
##########
@@ -1275,7 +1275,6 @@ protected void editOverwriteTest() {
}
@OptionCollectionTest.TestFunction
- @Override
Review Comment:
The class contains many unused (reported by IDE) methods - is that part of
the changes or can these methods be removed?
##########
apache-rat-core/src/main/java/org/apache/rat/commandline/Arg.java:
##########
@@ -625,64 +844,28 @@ private static List<String> processArrayFile(final
ArgumentContext context, fina
* @param context the context to process.
* @throws ConfigurationException if configuration files can not be read.
*/
- private static void processConfigurationArgs(final ArgumentContext
context) throws ConfigurationException {
- try {
- Defaults.Builder defaultBuilder = Defaults.builder();
- if (CONFIGURATION.isSelected()) {
- File[] files =
CONFIGURATION.getParsedOptionValues(context.getCommandLine());
- for (File file : files) {
- defaultBuilder.add(file);
- }
- }
- if (CONFIGURATION_NO_DEFAULTS.isSelected()) {
- // display deprecation log if needed.
-
context.getCommandLine().hasOption(CONFIGURATION_NO_DEFAULTS.getSelected());
- defaultBuilder.noDefault();
- }
- context.getConfiguration().setFrom(defaultBuilder.build());
+ private static void processConfigurationArgs(final ArgumentContext
context, final UIOptionCollection<?> optionCollection) throws
ConfigurationException {
- if (FAMILIES_APPROVED.isSelected()) {
-
context.getConfiguration().addApprovedLicenseCategories(processArrayArg(context,
FAMILIES_APPROVED));
- }
- if (FAMILIES_APPROVED_FILE.isSelected()) {
-
context.getConfiguration().addApprovedLicenseCategories(processArrayFile(context,
FAMILIES_APPROVED_FILE));
- }
- if (FAMILIES_DENIED.isSelected()) {
-
context.getConfiguration().removeApprovedLicenseCategories(processArrayArg(context,
FAMILIES_DENIED));
- }
- if (FAMILIES_DENIED_FILE.isSelected()) {
-
context.getConfiguration().removeApprovedLicenseCategories(processArrayFile(context,
FAMILIES_DENIED_FILE));
- }
-
- if (LICENSES_APPROVED.isSelected()) {
-
context.getConfiguration().addApprovedLicenseIds(processArrayArg(context,
LICENSES_APPROVED));
- }
+ Defaults.Builder defaultBuilder = Defaults.builder();
- if (LICENSES_APPROVED_FILE.isSelected()) {
-
context.getConfiguration().addApprovedLicenseIds(processArrayFile(context,
LICENSES_APPROVED_FILE));
- }
- if (LICENSES_DENIED.isSelected()) {
-
context.getConfiguration().removeApprovedLicenseIds(processArrayArg(context,
LICENSES_DENIED));
- }
-
- if (LICENSES_DENIED_FILE.isSelected()) {
-
context.getConfiguration().removeApprovedLicenseIds(processArrayFile(context,
LICENSES_DENIED_FILE));
- }
- if (COUNTER_MAX.isSelected()) {
- for (String arg :
context.getCommandLine().getOptionValues(COUNTER_MAX.getSelected())) {
- Pair<Counter, Integer> pair =
Converters.COUNTER_CONVERTER.apply(arg);
- int limit = pair.getValue();
-
context.getConfiguration().getClaimValidator().setMax(pair.getKey(), limit < 0
? Integer.MAX_VALUE : limit);
- }
- }
- if (COUNTER_MIN.isSelected()) {
- for (String arg :
context.getCommandLine().getOptionValues(COUNTER_MIN.getSelected())) {
- Pair<Counter, Integer> pair =
Converters.COUNTER_CONVERTER.apply(arg);
-
context.getConfiguration().getClaimValidator().setMin(pair.getKey(),
pair.getValue());
- }
- }
- } catch (Exception e) {
- throw ConfigurationException.from(e);
+ optionCollection.getSelected(CONFIGURATION).ifPresent(
+ selected -> {
+ File[] files = getParsedOptionValues(selected,
context.getCommandLine());
+ for (File file : files) {
+ defaultBuilder.add(file);
+ }
+ });
+
optionCollection.getSelected(CONFIGURATION_NO_DEFAULTS).ifPresent(selected -> {
+ // display deprecation log if needed.
+ context.getCommandLine().hasOption(selected);
+ defaultBuilder.noDefault();
+ });
+ context.getConfiguration().setFrom(defaultBuilder.build());
+
+ for (Arg arg : new Arg[]{FAMILIES_APPROVED, FAMILIES_APPROVED_FILE,
FAMILIES_DENIED, FAMILIES_DENIED_FILE,
Review Comment:
Personally I'd prefer List.of() due to better readability than arrays here
....
##########
apache-rat-plugin/src/test/resources/unit/it5/pom.xml:
##########
@@ -28,12 +28,12 @@
<version>@pom.version@</version>
<configuration>
<outputStyle>xml</outputStyle>
- <config>.rat/customConfig.xml</config>
- <inputExcludes>
- <exclude>.rat/**</exclude>
- <exclude>pom.xml</exclude>
- <exclude>invoker.properties</exclude>
- </inputExcludes>
+ <configs>
+ <config>.rat/customConfig.xml</config>
+ </configs>
+ <inputExcludes>.rat/**</inputExcludes>
Review Comment:
I'm not sure if this configuration is "Maven-style" - would it make sense to
ask over at the Maven mailing list?
plugins contains multiple plugin-entries .... that made me think about this
breaking change.
##########
apache-rat-core/src/main/java/org/apache/rat/ui/spi/UIProvider.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.rat.ui.spi;
+
+import org.apache.rat.ui.UI;
+import org.apache.rat.ui.UIOption;
+
Review Comment:
Should we add a general description here that explains the proposed
architecture?
--
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]
