Add support for easily creating ANSI messages from Commands

[bobtiernay-okta]

Something that would help ease the pain making awesome command lines is an abstraction over the console to help with creating formatted output within commands. There is great support for this for usage help. Why not extend this to the commands themselves? Think status reporting, warning and error messages.

I was thinking something like a Console, Terminal or CommandContext, etc. might be appropriate. This interface / implementation could encapsulate the state of the Ansi enum, the stdin / stout / stderr streams, and expose methods to write formatted tables, etc. Currently, such abstractions exist in the project, but are undocumented and not designed to be fluently consumed by CLI developers.

[remkop]

Interesting idea. There are some existing classes that are potentially candidates to be injected to support user commands during their execution, I'm thinking of CommandSpec and/or ParseResult, but perhaps a new abstraction is needed to also capture Ansi, and the streams like you point out.

I need to think about this some more but I like this idea.

[bobtiernay-okta]

Cool :) And as far as injection is concerned, I was also thinking this would be very handy at the highest levels, i.e. in the entry point. For example, this is what I'm currently doing:

public final class Main {

    public static void main(String[] args) {
        System.exit(new Main().run(args));
    }

    public int run(String... args) {
        try {
            Integer code = CommandLine.call(new AllCommand(), args);
            if (code == null) {
                return 1;
            }

            return code;
        } catch (ExecutionException e) {
            printError(e.getMessage());

            return 2;
        } catch (Throwable t) {
            printError(t);

            return 3;
        }
    }

    private static void printError(String message) {
        System.err.println(Ansi.ON.apply("Error running command: " + message, ImmutableList.of(Style.fg_red)).toString());
    }

    private void printError(Throwable t) {
        printError(Throwables.getStackTraceAsString(t));
    }

}

It would be nice to have something to aid at this level as well.

[remkop]

At that level, the caller is already in control of the Ansi instance and the streams. In your example, the following line:

CommandLine.call(new AllCommand(), args);

is the short form of:

CommandLine.call(new AllCommand(), System.out, System.err, Ansi.AUTO, args);

So, to expand your example:

public final class Main {

    public static void main(String[] args) {
        System.exit(new Main().run(args));
    }

    public int run(String... args) {
        PrintStream out = System.out;
        PrintStream err = System.err;
        Ansi ansi = Ansi.AUTO;
        try {
            Integer code = CommandLine.call(new AllCommand(), out, err, ansi, args);
            if (code == null) {
                return 1;
            }

            return code;
        } catch (ExecutionException e) {
            printError(err, ansi, e.getMessage());
            return 2;
        } catch (Throwable t) {
            printError(err, ansi, t);
            return 3;
        }
    }

    private static void printError(PrintStream stream, Ansi ansi, String message) {
        stream.println(ansi.new Text("@|red " + "Error running command: " + message + "|@").toString());
    }

    private void printError(PrintStream stream, Ansi ansi, Throwable t) {
        printError(stream, ansi, Throwables.getStackTraceAsString(t));
    }
}

(Note I used Ansi.new Text("@|style text|@") instead of the apply method.)

So at this level you are already okay, there is not much more that the picocli API could offer.
I thought you had in mind injecting the Ansi style and the streams into the command implementation so that the business logic could emit ANSI-styled text to the appropriate streams.

[bobtiernay-okta]

Correct, the only thing I was thinking is that things could be a bit more fluent at this level as well. But maybe this is sufficient as you point out. Thanks for the explanation!

Btw, why was the decision to not throw ParameterExceptions made for the version of call? It seems to cause inconsistencies in the caller as you can see above.

[remkop]

Both the run and the call convenience methods are implemented as:

IParseResultHandler2 handler = new RunLast();
IExceptionHandler2 exceptionHandler = new DefaultExceptionHandler<List<Object>>();

ParseResult parseResult = null;
try {
    parseResult = parseArgs(args);
    return handler.handleParseResult(parseResult);
} catch (ParameterException ex) {
    return exceptionHandler.handleParseException(ex, (String[]) args);
} catch (ExecutionException ex) {
    return exceptionHandler.handleExecutionException(ex, parseResult);
}

RunLast internally calls execute:

private static List<Object> execute(CommandLine parsed, List<Object> executionResult) {
    Object command = parsed.getCommand();
    if (command instanceof Runnable) {
        try {
            ((Runnable) command).run();
            executionResult.add(null); // for compatibility with picocli 2.x
            return executionResult;
        } catch (ParameterException ex) {
            throw ex;
        } catch (ExecutionException ex) {
            throw ex;
        } catch (Exception ex) {
            throw new ExecutionException(parsed, "Error while running command (" + command + "): " + ex, ex);
        }
    } else if (command instanceof Callable) {
        // ... similar
    }
    throw new ExecutionException(parsed, "Parsed command (" + command + ") is not Runnable or Callable");
}

So unless you really need to handle Throwable instead of Exception, you may want to move your exception handling logic in a custom IExceptionHandler2 implemementation.

[bobtiernay-okta]

I guess my question is, as a user I would expect to have the following semantics by default:

private static class DefaultExceptionHandler2 extends DefaultExceptionHandler {

        @Override
        public Object handleParseException(ParameterException ex, String[] args) {
            err().println(ex.getMessage());
            ex.getCommandLine().usage(err(), ansi());
            throw ex;
        }

    }

so that this would be possible:

  public int run(String... args) {
        try {
            return CommandLine.call(new AllCommand(), args);
        } catch (ParameterException e) {
            // Or use e to determine return code
            return 1;
        } catch (ExecutionException e) {
            printError(e.getMessage());
            // Or use e with instanceof checking
            return 2;
        } catch (MyServiceException e) {
            printError(t);

            return 3;
        }
    }

The reason is that it makes handling the return codes uniform by default. It is very common to want to return a non-zero return code in this case. Right now, it it seems unnatural to detect that for both the Runnable and Callable cases.

I also wish that ExecutionException didn't wrap my business logic exceptions as it causes unnatural instanceof checks and casting in that branch, but I understand that gives the ability to have the CommandLine object in scope.

Somewhat related is that I strongly believe that calling System.exit should be at the highest level of the call stack so that testing is simplified. That's why I don't use this support in Picocli.

I guess what I'm getting at is that having the defaults aligned with testability and principle of least surprise would be nice.

[remkop]

The objective of the run and call convenience methods is to allow applications to execute with a single line of code (including some reasonable exception handling).

The semantics you are looking for are available with the parse and parseArgs methods. These methods require the caller to do the exception handling.

You can still easily run the last subcommand by creating a new RunLast() instance and letting it handle the parse result. Something like this:

try {
  ParseResult result = new CommandLine(new AllCommands()).parseArgs(args);
  new RunLast().handleParseResult(result);
} catch (ParameterException ex) {
  // ...
} catch (ExecutionException ex) {
  // ...
}

[bobtiernay-okta]

Ok, I think I'm better understanding the intent of these objects now. Thanks for clarifying with these examples.

One thought I had is that the "convenience" methods remind me a lot of how Spring Boot expects users to create their entry points. For example, here's the analog of CommandLine.run:

   public static void main(String[] args) {
        SpringApplication.run(Application.class, args);
    }

However, they also have a builder that is somewhat more verbose, yet still declarative, and allows customization:

   public static void main(String[] args) {
      new SpringApplicationBuilder()
		.sources(Parent.class)
		.child(Application.class)
		.bannerMode(Banner.Mode.OFF)
		.run(args);
   }

Perhaps it is this sweet spot that I'm after. Somewhere between implementing interfaces on one end and a single line on the other, that I feel that Picocli could benefit from.

[remkop]

Interesting, thanks for the reference!

[remkop]

One thing that may be interesting for testing: I've started to use Stephan Birkner's SystemErrAndOutRule and ExpectedSystemExit rule. This works very well for JUnit 4.

If you using JUnit 5, and you need this urgently, you can offer Stefan to help with his work in progress on a JUnit 5 extension.

[remkop]

I'm thinking to add the following methods to the Ansi enum class:

/**
 * Returns a new Text object for this Ansi mode, encapsulating the specified string
 * which may contain markup like {@code @|bg(red),white,underline some text|@}.
 * <p>
 * Calling {@code toString()} on the returned Text will either include ANSI escape codes
 * (if this Ansi mode is ON), or suppress ANSI escape codes (if this Ansi mode is OFF).
 * <p>
 * Equivalent to {@code this.new Text(stringWithMarkup)}.
 */ 
Text text(String stringWithMarkup);

/**
 * Returns a String where any markup like 
 * {@code @|bg(red),white,underline some text|@} is converted to ANSI escape codes 
 * if this Ansi is ON, or suppressed if this Ansi is OFF.
 * <p>
 * Equivalent to {@code this.new Text(stringWithMarkup).toString()}.
 */ 
String string(String stringWithMarkup);

/** Returns Ansi.ON if enabled is true, Ansi.OFF otherwise. */
static Ansi valueOf(boolean enabled); // convenience method