Refactor CLI to remove --severity from some commands

Some commands don't need the severity option and should instead only show
validation events if the command fails. This includes the ast and select
commands. To make this work, I refactored several CLI classes:

* Add a shared ModeBuilder abstraction to cleanup the old utility code. This
  class now uses a Validator.Mode to indicate how validation is reported,
  decoupling arguments from the shared validation abstraction.
* Move --discover and --discover-classpath to DiscoveryOptions
* Move --severity to SeverityOption so it's more granular
* Add the ability to check for receivers on Arguments
* Add the ability to remove receivers from Arguments
* Add the ability to get positional arguments more than once
* Can now add/remove argument receivers in ClasspathCommand subclasses

smithy-cli/src/it/java/software/amazon/smithy/cli/AstCommandTest.java

@@ -0,0 +1,53 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file 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 software.amazon.smithy.cli;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+
+import org.junit.jupiter.api.Test;
+import software.amazon.smithy.utils.ListUtils;
+
+public class AstCommandTest {
+    @Test
+    public void validatesModelSuccess() {
+        IntegUtils.run("model-with-warning", ListUtils.of("ast"), result -> {
+            assertThat(result.getExitCode(), equalTo(0));
+            assertThat(result.getOutput(), containsString("\"smithy\""));
+            assertThat(result.getOutput(), not(containsString("WARNING")));
+        });
+    }
+
+    @Test
+    public void convertsSyntacticallyCorrectModels() {
+        IntegUtils.run("invalid-model", ListUtils.of("ast"), result -> {
+            assertThat(result.getExitCode(), equalTo(0));
+            assertThat(result.getOutput(), containsString("\"smithy\""));
+            assertThat(result.getOutput(), not(containsString("WARNING")));
+        });
+    }
+
+    @Test
+    public void showsErrorsForSyntacticallyIncorrectModels() {
+        IntegUtils.run("model-with-syntax-error", ListUtils.of("ast"), result -> {
+            assertThat(result.getExitCode(), equalTo(1));
+            assertThat(result.getOutput(), containsString("ERROR"));
+            assertThat(result.getOutput(), containsString("bar // <- invalid syntax"));
+        });
+    }
+}

smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/model/main.smithy

@@ -0,0 +1,6 @@
+$version: "2.0"
+namespace smithy.example
+
+structure Foo {
+    bar // <- invalid syntax
+}

smithy-cli/src/it/resources/software/amazon/smithy/cli/projects/model-with-syntax-error/smithy-build.json

@@ -0,0 +1,4 @@
+{
+    "version": "1.0",
+    "sources": ["model"]
+}

smithy-cli/src/main/java/software/amazon/smithy/cli/Arguments.java

@@ -27,9 +27,9 @@
  * Command line arguments list to evaluate.
  *
  * <p>Arguments are parsed on demand. To finalize parsing arguments and
- * force validation of remaining arguments, call {@link #finishParsing()}.
+ * force validation of remaining arguments, call {@link #getPositional()}.
  * Note that because arguments are parsed on demand and whole set of
- * arguments isn't known before {@link #finishParsing()} is called, you
+ * arguments isn't known before {@link #getPositional()} is called, you
  * may need to delay configuring parts of the CLI or commands by adding
  * subscribers via {@link #onComplete(BiConsumer)}. These subscribers are
  * invoked when all arguments have been parsed.
@@ -39,6 +39,7 @@ public final class Arguments {
     private final String[] args;
     private final Map<Class<? extends ArgumentReceiver>, ArgumentReceiver> receivers = new LinkedHashMap<>();
     private final List<BiConsumer<Arguments, List<String>>> subscribers = new ArrayList<>();
+    private List<String> positional;
     private boolean inPositional = false;
     private int position = 0;
 
@@ -55,6 +56,28 @@ public <T extends ArgumentReceiver> void addReceiver(T receiver) {
         receivers.put(receiver.getClass(), receiver);
     }
 
+    /**
+     * Removes an argument receiver by class.
+     *
+     * @param receiverClass Class of receiver to remove.
+     * @return Returns the removed receiver if found, or null.
+     * @param <T> Kind of receiver to remove.
+     */
+    @SuppressWarnings("unchecked")
+    public <T extends ArgumentReceiver> T removeReceiver(Class<T> receiverClass) {
+        return (T) receivers.remove(receiverClass);
+    }
+
+    /**
+     * Check if this class contains a receiver.
+     *
+     * @param receiverClass Class of receiver to detect.
+     * @return Returns true if found.
+     */
+    public boolean hasReceiver(Class<? extends ArgumentReceiver> receiverClass) {
+        return receivers.containsKey(receiverClass);
+    }
+
     /**
      * Get a receiver by class.
      *
@@ -97,7 +120,7 @@ public boolean hasNext() {
     }
 
     /**
-     * Peeks at the next value in the arguments list without shifting it.
+     * Peeks at the next value in the argument list without shifting it.
      *
      * <p>Note that arguments consumed by a {@link ArgumentReceiver} are never
      * peeked.
@@ -134,7 +157,7 @@ public String peek() {
     }
 
     /**
-     * Shifts off the next value in the arguments list or returns null.
+     * Shifts off the next value in the argument list or returns null.
      *
      * @return Returns the next value or null.
      */
@@ -166,41 +189,41 @@ public String shiftFor(String parameter) {
     }
 
     /**
-     * Expects that all remaining arguments are positional, and returns them.
+     * Gets the positional arguments.
+     *
+     * <p>Expects that all remaining arguments are positional, and returns them.
      *
      * <p>If an argument is "--", then it's skipped and remaining arguments are
      * considered positional. If any argument is encountered that isn't valid
      * for a registered Receiver, then an error is raised. Otherwise, all remaining
      * arguments are returned in a list.
      *
-     * <p>Subscribers for different receivers are called when this method is called.
+     * <p>Subscribers for different receivers are called when this method is first called.
      *
      * @return Returns remaining arguments.
      * @throws CliError if the next argument starts with "--" but isn't "--".
      */
-    public List<String> finishParsing() {
-        List<String> positional = new ArrayList<>();
-
-        while (hasNext()) {
-            String next = shift();
-            if (next != null) {
-                if (!inPositional && next.startsWith("-")) {
-                    throw new CliError("Unexpected CLI argument: " + next);
-                } else {
-                    inPositional = true;
-                    positional.add(next);
+    public List<String> getPositional() {
+        if (positional == null) {
+            positional = new ArrayList<>();
+
+            while (hasNext()) {
+                String next = shift();
+                if (next != null) {
+                    if (!inPositional && next.startsWith("-")) {
+                        throw new CliError("Unexpected CLI argument: " + next);
+                    } else {
+                        inPositional = true;
+                        positional.add(next);
+                    }
                 }
             }
-        }
 
-        invokeSubscribers(positional);
+            for (BiConsumer<Arguments, List<String>> subscriber : subscribers) {
+                subscriber.accept(this, positional);
+            }
+        }
 
         return positional;
     }
-
-    private void invokeSubscribers(List<String> positional) {
-        for (BiConsumer<Arguments, List<String>> subscriber : subscribers) {
-            subscriber.accept(this, positional);
-        }
-    }
 }

smithy-cli/src/main/java/software/amazon/smithy/cli/commands/AstCommand.java

@@ -22,6 +22,7 @@
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.shapes.ModelSerializer;
+import software.amazon.smithy.model.validation.Severity;
 
 final class AstCommand extends ClasspathCommand {
 
@@ -39,9 +40,26 @@ public String getSummary() {
         return "Reads Smithy models in and writes out a single JSON AST model.";
     }
 
+    @Override
+    protected void configureArgumentReceivers(Arguments arguments) {
+        super.configureArgumentReceivers(arguments);
+
+        // The AST command isn't meant for validation. Events are only shown when they fail the command.
+        arguments.removeReceiver(SeverityOption.class);
+    }
+
     @Override
     int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, List<String> models) {
-        Model model = CommandUtils.buildModel(arguments, models, env, env.stderr(), ValidationFlag.QUIET, config);
+        Model model = new ModelBuilder()
+                .config(config)
+                .arguments(arguments)
+                .env(env)
+                .models(models)
+                .validationPrinter(env.stderr())
+                .validationMode(Validator.Mode.QUIET)
+                .severity(Severity.DANGER)
+                .build();
+
         ModelSerializer serializer = ModelSerializer.builder().build();
         env.stdout().println(Node.prettyPrintJson(serializer.serialize(model)));
         return 0;

smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildCommand.java

@@ -89,8 +89,9 @@ public void registerHelp(HelpPrinter printer) {
     }
 
     @Override
-    protected void addAdditionalArgumentReceivers(List<ArgumentReceiver> receivers) {
-        receivers.add(new Options());
+    protected void configureArgumentReceivers(Arguments arguments) {
+        super.configureArgumentReceivers(arguments);
+        arguments.addReceiver(new Options());
     }
 
     @Override
@@ -99,11 +100,13 @@ int runWithClassLoader(SmithyBuildConfig config, Arguments arguments, Env env, L
         BuildOptions buildOptions = arguments.getReceiver(BuildOptions.class);
         StandardOptions standardOptions = arguments.getReceiver(StandardOptions.class);
         ClassLoader classLoader = env.classLoader();
-
-        // Build the model and fail if there are errors. Prints errors to stdout.
-        // Configure whether the build is quiet or not based on the --quiet option.
-        ValidationFlag flag = ValidationFlag.from(standardOptions);
-        Model model = CommandUtils.buildModel(arguments, models, env, env.stderr(), flag, config);
+        Model model = new ModelBuilder()
+                .config(config)
+                .arguments(arguments)
+                .env(env)
+                .models(models)
+                .validationPrinter(env.stderr())
+                .build();
 
         Supplier<ModelAssembler> modelAssemblerSupplier = () -> {
             ModelAssembler assembler = Model.assembler(classLoader);

smithy-cli/src/main/java/software/amazon/smithy/cli/commands/BuildOptions.java

@@ -17,108 +17,49 @@
 
 import java.util.function.Consumer;
 import software.amazon.smithy.cli.ArgumentReceiver;
-import software.amazon.smithy.cli.CliError;
 import software.amazon.smithy.cli.HelpPrinter;
-import software.amazon.smithy.cli.StandardOptions;
-import software.amazon.smithy.model.validation.Severity;
 
 /**
  * Arguments available to commands that load and build models.
  */
 final class BuildOptions implements ArgumentReceiver {
 
-    static final String SEVERITY = "--severity";
     static final String ALLOW_UNKNOWN_TRAITS = "--allow-unknown-traits";
     static final String MODELS = "<MODELS>";
 
-    private Severity severity;
-    private String discoverClasspath;
     private boolean allowUnknownTraits;
-    private boolean discover;
     private String output;
 
     @Override
     public void registerHelp(HelpPrinter printer) {
-        printer.param(SEVERITY, null, "SEVERITY", "Set the minimum reported validation severity (one of NOTE, "
-                                                  + "WARNING [default setting], DANGER, ERROR).");
         printer.option(ALLOW_UNKNOWN_TRAITS, null, "Ignore unknown traits when validating models");
-        /*
-        Hide these for now until we figure out a plan forward for these.
-        printer.option(DISCOVER, "-d", "Enable model discovery, merging in models found inside of jars");
-        printer.param(DISCOVER_CLASSPATH, null, "CLASSPATH",
-                            "Enable model discovery using a custom classpath for models");
-        */
         printer.param("--output", null, "OUTPUT_PATH",
                       "Where to write Smithy artifacts, caches, and other files (defaults to './build/smithy').");
         printer.positional(MODELS, "Model files and directories to load");
     }
 
     @Override
     public boolean testOption(String name) {
-        switch (name) {
-            case ALLOW_UNKNOWN_TRAITS:
-                allowUnknownTraits = true;
-                return true;
-            case "--discover":
-            case "-d":
-                discover = true;
-                return true;
-            default:
-                return false;
+        if (ALLOW_UNKNOWN_TRAITS.equals(name)) {
+            allowUnknownTraits = true;
+            return true;
         }
+        return false;
     }
 
     @Override
     public Consumer<String> testParameter(String name) {
-        switch (name) {
-            case "--output":
-                return value -> output = value;
-            case "--discover-classpath":
-                return value -> discoverClasspath = value;
-            case SEVERITY:
-                return value -> {
-                    severity(Severity.fromString(value).orElseThrow(() -> {
-                        return new CliError("Invalid severity level: " + value);
-                    }));
-                };
-            default:
-                return null;
+        if ("--output".equals(name)) {
+            return value -> output = value;
         }
-    }
-
-    String discoverClasspath() {
-        return discoverClasspath;
+        return null;
     }
 
     boolean allowUnknownTraits() {
         return allowUnknownTraits;
     }
 
-    boolean discover() {
-        return discover;
-    }
-
     String output() {
         return output;
     }
-
-    void severity(Severity severity) {
-        this.severity = severity;
-    }
-
-    /**
-     * Get the severity level, taking into account standard options that affect the default.
-     *
-     * @param options Standard options to query if no severity is explicitly set.
-     * @return Returns the resolved severity option.
-     */
-    Severity severity(StandardOptions options) {
-        if (severity != null) {
-            return severity;
-        } else if (options.quiet()) {
-            return Severity.DANGER;
-        } else {
-            return Severity.WARNING;
-        }
-    }
 }