Introduce new command builder DSL

[piotrooo]

Below is a new, improved DSL that addresses #1127. You’ll find the usage of individual methods (all of them are listed) to make it easier to review everything clearly.

CommandRegistration.builder()
	.command("command")
	.description("description")
	.group("group")

	.interactionMode(InteractionMode.NONINTERACTIVE)
	.isInteractive()
	.isNonInteractive()
	.availability(Availability::available)

	.hidden(true)
	.hidden()

	.withOption(option -> option
			.longNames("arg")
			.shortNames('a')

			.type(String.class)
			.type(ResolvableType.forType(String.class))

			.description("some arg")
			.label("label")
			.defaultValue("defaultValue")
			.nameModifier(String::toUpperCase)

			.required()
			.required(true)

			.position(0)

			.arity(1, 2)
			.arity(OptionArity.ZERO_OR_MORE)

			.completion(completionContext -> List.of())
	)
	.defaultOptionNameModifier(String::toLowerCase)

	.withAlias(alias -> alias
			.command("alias")
			.group("Alias Group")
	)
	.withTarget(target -> target
			.function(function -> function)
			.consumer(commandContext -> {})

			.method(new Object(), "method", String.class)
			.method(new Object(), "method")
	)
	.withExitCode(exitCode -> exitCode
			.map(RuntimeException.class, 1)
			.map(throwable -> 2)
	)
	.withErrorHandling(errorHandling -> errorHandling
			.resolver(ex -> CommandHandlingResult.of("msg"))
	)

	.build();

Important

Since command registration is the heart of Spring Shell, it's very important to carefully review the contracts of the introduced changes. Modifying this API in the future may be problematic, so we need to be sure that this structure is acceptable, as it may stay with us for a long time.

However, when I look at the resulting code — and the existing code in the codebase — I'm not entirely satisfied. The code feels quite clumsy. 😭

I have a few topics I'd like to bring up.

Is the naming of these objects good?

I see an analogy here to BeanDefinition and BeanDefinitionRegistry. Right now, we have the name CommandRegistration. But what exactly is this mysterious Registration? I have the impression that it's not a good name. It’s not very descriptive and might be misleading.
Maybe we should approach it similarly to the Spring Framework itself and have definitions and registries for commands?

In the definition, we could have a command object with all its metadata — such as names, aliases, and options.

Should we use interfaces?

I'm not sure what the benefit is of having CommandRegistration as an interface (and the rest of objects too). I don't really feel that this approach fits well.

If we had multiple implementations (1..n) of different CommandRegistrations, then an interface might make sense. Here, it seems unnecessary.

Another question, how should the objects that hold command definition information look?

Should we use the Spec objects everywhere, or only during the build phase — where the result would be an object holder?
For example: OptionSpec could create an OptionDefinition object (which could be a simple record).

Aggressiveness of changes

I'm also not sure how far-reaching these changes should be. We're changing the baseline — but are the modifications I’ve introduced acceptable? Could we possibly discuss further, larger changes? Do we need to maintain backward compatibility?

Summary

Overall, this image perfectly captures how I feel when walking through the classes and trying to understand how commands are created.

Caution

Without good and clear manual command registration, it will be very difficult to build reliable automatic detection and registration later on.

I'd be happy 🙏 to continue driving this topic, but I need answers to the above questions so I don't go down a dead-end path — and to know how far I can go with the refactor.

[fmbenhassine]

Thank you for the PR!

I see an analogy here to BeanDefinition and BeanDefinitionRegistry. Right now, we have the name CommandRegistration. But what exactly is this mysterious Registration? I have the impression that it's not a good name. It’s not very descriptive and might be misleading.
Maybe we should approach it similarly to the Spring Framework itself and have definitions and registries for commands?

I agree, the name is confusing (naming is hard..). I recently renamed CommandCatalog to CommandRegistry (see 3d4dce2). So If we rename Command to CommandDefinition, we would end up with CommandDefinition and CommandDefinitionRegistry just like BeanDefinition and BeanDefinitionRegistry (which is a good thing).

However, as mentioned in #1209 (reply in thread), we might not need to rename Command to CommandDefinition, Command is already a good name and very appealing as you mentioned, so it can be well used in a CommandRegistry (after the recent rename). Do you agree?

We can still use CommandDefinition (similar to CommandSpec in Picocli for example) with the DSL to create Command objects. But that is a nice-to-have feature, not the single entry point.

I think if we redesign Command to contain a command definition (and not just be an empty interface), we are good to go to use that in place of CommandRegistration.

I'm not sure what the benefit is of having CommandRegistration as an interface (and the rest of objects too). I don't really feel that this approach fits well. If we had multiple implementations (1..n) of different CommandRegistrations, then an interface might make sense. Here, it seems unnecessary.

Yes, I am thinking of introducing a new programming model based on Command and ditch CommandRegistry altogether (but I need to try that beforehand, you can give it a try if you want as well).

Another question, how should the objects that hold command definition information look?

I think of something as simple as:

public interface Command {

    String getName();
    String getDescription();

    // other command properties
  
    // a method to define the business logic of the command
    // input/output types can be reviewed here, I am thinking of CommandExitCode ?
    String execute(String[] args) throws Exception;

}

Should we use the Spec objects everywhere, or only during the build phase — where the result would be an object holder? For example: OptionSpec could create an OptionDefinition object (which could be a simple record).

No need for Specs and Definitions, only Definitions. And that would be only used by the DSL.

I'm also not sure how far-reaching these changes should be. We're changing the baseline — but are the modifications I’ve introduced acceptable? Could we possibly discuss further, larger changes? Do we need to maintain backward compatibility?

Yes, the modifications you introduced are acceptable, and there is no need to maintain backward compatibility since we are designing a major release.

[piotrooo]

@fmbenhassine, I'd like to thank you very much for such detailed feedback.

Let me summarize all the information so far.

The @Command annotation remains and is used as before (of course, we need to fix a few things related to registration and discovery).

A CommandRegistry has been introduced as a central place where all defined commands are stored. This registry contains Command objects. The @Command annotation causes the creation of a Command object.

Here’s an example of what a Command object could look like (simplified, without all fields/methods for clarity):

class Command {
    private final String name;
    public Command(String name) {}
    // ...
}

It's an object (not an interface?) that would contain all information about the command.

The first question: do you think the name conflict between the annotation and the class Command could be confusing? I don't think so, but what do you think?

No need for Specs and Definitions, only Definitions. And that would be only used by the DSL.

So maybe it could look something like this:

Command.builder()
    .name("name")
    .description("desc")
    .withOption(option -> …)
    .build()

The build() method would create a new Command() with the filled-in values. I think we should enforce building either through the builder or the annotation. How do you feel about that?

Finally, the user will have two ways to register a command.

Through the @Command annotation

class SomeCommand {
    @Command
    String test() {
        return "test";
    }
}

Manually through the builder

@Bean
Command someCommand() {
    return Command.builder()
        .name("name")
        .description("desc")
        .withOption(option -> …)
        .build();
}

I'm also thinking about another approach, though it would require changing the concept a bit.

What about cases where we have classes that implement the Command interface (in that case, the example above with creating a Command object should probably change to something like CommandDefinition)?

Something like:

class SomeCommand implements Command {}

Such classes could also be auto-discovered and added to the registry. Semantically, it would mean that it’s a single command.

In that case, the Command interface would have an execute() method. However, I'm wondering what about the remaining methods describing options, description, etc. I think it's worth considering and trying to address this.

In your example regarding the command, you introduced an execute() method. Do you think it's a good idea to combine definition and execution? Maybe these should be two separate types, clearly divided to establish boundaries between these two entities.

And what’s your view on merging this PR? Should I continue working here, or would you prefer to merge these changes now and treat them somewhat independently?

[fmbenhassine]

You are welcome! Thank YOU for all your time and effort on this.

Some feedback on your input (as I am currently experimenting with all this):

Regarding command registry:

The @command annotation remains and is used as before (of course, we need to fix a few things related to registration and discovery).
A CommandRegistry has been introduced as a central place where all defined commands are stored. This registry contains Command objects. The @command annotation causes the creation of a Command object.

Correct. I already changed CommandRegistry to be a .. command registry:

public class CommandRegistry {

    private Set<Command> commands;

   // methods like register, unregister, etc

}

Note it is a class, not an interface anymore (I really see no need for it to be an interface, KISS!).

Regarding commands:

The first question: do you think the name conflict between the annotation and the class Command could be confusing? I don't think so, but what do you think?

I don't think so neither, so I agree with you and I don't see this as a source of confusion, especially that they live in different packages.

You asked about Command being a class vs an interface:

Here’s an example of what a Command object could look like (simplified, without all fields/methods for clarity):

class Command {
    private final String name;
    public Command(String name) {}
    // ...
}
It's an object (not an interface?) that would contain all information about the command.

[...]

What about cases where we have classes that implement the Command interface (in that case, the example above with creating a Command object should probably change to something like CommandDefinition)?

In my opinion, Command should be an interface a user can implement to contribute commands to the shell framework. In a Spring world, we should be able to contribute commands by defining them as beans in the application context:

@Bean
public Command hello() {
   return new HelloCommand(); // HelloCommand implements Command
}

A bit similar to what we do with other projects from the portfolio like batch or integration:

@Bean
public Job job() {
   return new MyBatchJob(); // MyBatchJob implements Job
}

That's the foundational programming model that serves as a base for 1) the annotation model and 2) the DSL.

1. For the annotation model, and with all the power of Spring (proxies, SpEL, etc), we should be able to turn a method into a command as you mentioned "The @command annotation causes the creation of a Command object."

@Command
String test() {
   return "test";
}

2. For the DSL, we can indeed have a basic implementation of Command that we construct with a Builder. The snippet you suggest looks perfect to me! We only have to provide the user with a functional interface to define the business logic of the command (and avoid to implement the entire interface) that the builder accepts, more on this in the next section.

Regarding Command execution

In that case, the Command interface would have an execute() method. However, I'm wondering what about the remaining methods describing options, description, etc. I think it's worth considering and trying to address this.

In your example regarding the command, you introduced an execute() method. Do you think it's a good idea to combine definition and execution? Maybe these should be two separate types, clearly divided to establish boundaries between these two entities.

They are already two separate types: We have Command and CommandExecution already (and even a CommandContext and a CommandExitCode). But they are all defined in a "non natural" way (IMHO). Here is what I am working on right now (keeping things as simple as possible):

public interface Command {
    String getName();
    String getDescription();
    String getHelp();
    String getGroup();
    List<CommandOption> getOptions();
    List<CommandAlias> getAliases();

    /**
     * Execute the command within the given context.
     * @param commandContext the context of the command
     * @return the result of the command execution
     */
    CommandExecution execute(CommandContext commandContext);

}

Domain types are now defined as records:

/**
 * Record representing the execution of a command.
 */
public record CommandExecution(LocalDateTime startTime, LocalDateTime endTime, CommandExitCode exitCode) {
}

/**
 * Record representing the execution of a command.
 */
public record CommandExitCode(int exitCode, String exitDescription, List<Throwable> errors) {
}

/**
 * Record containing information about current command execution.
 */
public record CommandContext(String[] rawArgs, Terminal terminal) {

}

Optional, nice to have functional interface to define commands logic

With that in place, and in order to avoid making users implement the entire Command interface, we can define a base implementation of Command with a functional interface (we need to find a good name) to define the command's logic (similar to the Tasklet interface in Spring Batch):

@FunctionalInterface
public interface CommandXXX { // find a good name

    /**
     * Execute the command within the given context.
     * @param commandContext the context of the command
     * @return the result of the command execution
     */
    CommandExecution execute(CommandContext commandContext);

}

This makes it possible to write something like this with the DSL:

@Bean
Command someCommand() {
    return Command.builder()
        .name("name")
        .description("desc")
        .withOption(option -> …)
        .execute(context -> System.out.println("hello"))
        .build();
}

What do you think? I find this to be simplistic design, more "natural" than the current way of doing things and consistent with the rest of the portfolio. But I am open to a second thought, I might be missing some details!

Another possible design is to define that functional interface as CommandExecution (and remove the CommandExecution and CommandExitCode records I mentioned about above, which is a simpler design):

@FunctionalInterface
public interface CommandExecution {

    /**
     * Execute the command within the given context.
     * @param commandContext the context of the command
     */
    void execute(CommandContext commandContext) throws Exception;

}

The idea here is that since the CommandContext provides access to the terminal, the command implementation can use that to print things and return, or throw an exception to signal any error to the framework (notice the void return type in this case)

And what’s your view on merging this PR? Should I continue working here, or would you prefer to merge these changes now and treat them somewhat independently?

I am currently working on the new design of the command definition, registration and discovery (and more deeply, how to configure Spring Shell with @EnableCommand without Boot). Since this PR is about the DSL, I would suggest you hold on until we have a solid foundation (I see the DSL as a nice to have feature, but I am still targeting this PR for v4).

I will drop a message here and in other PRs when main is ready again.

[piotrooo]

In my opinion, Command should be an interface a user can implement to contribute commands to the shell framework. In a Spring world, we should be able to contribute commands by defining them as beans in the application context:

After some refactoring and playing around with the code, I agree.

Domain types are now defined as records:

Haha, sorry — this is just how my refactors go. Are you reading my mind? 🤔 Records feel more natural for simple data holders.

With that in place, and in order to avoid making users implement the entire Command interface, we can define a base implementation of Command with a functional interface (we need to find a good name) to define the command's logic (similar to the Tasklet interface in Spring Batch):

Yes, that syntactic sugar will be super helpful for small tools.

[piotrooo]

@fmbenhassine you can check how my refactor is going.

[fmbenhassine]

It is going in the right direction! However, I am in the middle of a more aggressive refactoring, so apologies upfront but you will have to rebase your PR 😔 By aggressive I mean a lot of big breaking changes: removal of CommandResolver (#1217), CommandExceptionResolver, ResultHandler, the Shell class (see #1218), etc. So please expect those changes on rebase. Moreover, I still have a lot of questions: why is option arity an enum? InteractionMode will be obsolete when resolving #1218, "hidden" commands are a big question mark to me as well (why it's even there if we want to hide it..).

In this PR, I find Command.Builder very nicely done 👍 I would have done it exactly the same way. I think DefaultCommand is redundant, we have AbstractCommand that we can reuse (we can change it completely if needed). Moreover, I talked about adding a new functional interface for command execution, in hindsight, no need for that. I was able to make Command itself a functional interface by using default methods:

@FunctionalInterface
public interface Command {

    /**
     * Get the name of the command.
     * @return the name of the command
     */
    default String getName() {
        return this.getClass().getSimpleName().toLowerCase();
    }

    /**
     * Get a short description of the command.
     * @return the description of the command
     */
    default String getDescription() {
        return "";
    }

    /**
     * Get the help text of the command.
     * @return the help text of the command
     */
    default String getHelp() {
        // TODO generate default help from description, options, aliases, etc.
        return "";
    }

    /**
     * Get the group of the command.
     * @return the group of the command
     */
    default String getGroup() {
        return "";
    }

    /**
     * Get the options of the command.
     * @return the options of the command
     */
    default List<CommandOption> getOptions() {
        return Collections.emptyList();
    }

    /**
     * Get the aliases of the command.
     * @return the aliases of the command
     */
    default List<CommandAlias> getAliases() {
        return Collections.emptyList();
    }

    // TODO should command availability be defined here on in the CommandContext (some commands should be available or not depending on the context)

    /**
     * Execute the command within the given context.
     * @param commandContext the context of the command
     */
    void execute(CommandContext commandContext) throws Exception;

}

Unfortunately, it will be very hard to do this in an incremental way with distinct "clean" commits that build correctly (currently everything is tied together), so I will make the exception and push changes to main even in a non stable form. In order to avoid duplicate efforts and waste time, I would suggest you to check the changes to get an idea but not to rebase your PR until I make main quite stable again. I will do my best to make this as short as possible and let you know when done.

Keep tuned!

[piotrooo]

It is going in the right direction! However, I am in the middle of a more aggressive refactoring, so apologies upfront but you will have to rebase your PR 😔 By aggressive I mean a lot of big breaking changes: removal of CommandResolver (#1217), CommandExceptionResolver, ResultHandler, the Shell class (see #1218), etc. So please expect those changes on rebase.

Okay, then I’ll wait for those.

Moreover, I still have a lot of questions: why is option arity an enum? InteractionMode will be obsolete when resolving #1218, "hidden" commands are a big question mark to me as well (why it's even there if we want to hide it..).

Baby steps — first, make a working example with the basic stuff, and also gather some feedback.

In this PR, I find Command.Builder very nicely done 👍 I would have done it exactly the same way. I think DefaultCommand is redundant, we have AbstractCommand that we can reuse (we can change it completely if needed).

I'm not quite sure it's the right place, but maybe I don't fully understand your intentions. I like the Default pattern — it’s similar to, for example, RestClient / WebClient or ChatClient from spring-ai.

Moreover, I talked about adding a new functional interface for command execution, in hindsight, no need for that. I was able to make Command itself a functional interface by using default methods:

@FunctionalInterface

public interface Command {



    /**

     * Get the name of the command.

     * @return the name of the command

     */

    default String getName() {

        return this.getClass().getSimpleName().toLowerCase();

    }



    /**

     * Get a short description of the command.

     * @return the description of the command

     */

    default String getDescription() {

        return "";

    }



    /**

     * Get the help text of the command.

     * @return the help text of the command

     */

    default String getHelp() {

        // TODO generate default help from description, options, aliases, etc.

        return "";

    }



    /**

     * Get the group of the command.

     * @return the group of the command

     */

    default String getGroup() {

        return "";

    }



    /**

     * Get the options of the command.

     * @return the options of the command

     */

    default List<CommandOption> getOptions() {

        return Collections.emptyList();

    }



    /**

     * Get the aliases of the command.

     * @return the aliases of the command

     */

    default List<CommandAlias> getAliases() {

        return Collections.emptyList();

    }



    // TODO should command availability be defined here on in the CommandContext (some commands should be available or not depending on the context)



    /**

     * Execute the command within the given context.

     * @param commandContext the context of the command

     */

    void execute(CommandContext commandContext) throws Exception;



}

Hmm, maybe that's a good idea. But are you thinking about defaults for all methods?

Unfortunately, it will be very hard to do this in an incremental way with distinct "clean" commits that build correctly (currently everything is tied together), so I will make the exception and push changes to main even in a non stable form. In order to avoid duplicate efforts and waste time, I would suggest you to check the changes to get an idea but not to rebase your PR until I make main quite stable again. I will do my best to make this as short as possible and let you know when done.

Okay, so please give me a heads-up when you're done ✅

[fmbenhassine]

Baby steps — first, make a working example with the basic stuff, and also gather some feedback.

Sure! I pushed the basic building blocks for the new programming model, see 7199ea4. The hello world sample now runs as expected with the new design described in #1207 (notice the two sample commands, one to showcase the annotation model and the other to showcase the programmatic model).

As mentioned previously, the plan is to iterate on this. There are a couple TODOs like options handling, aliases, and command availability. Once that in place, I think we can tackle the new DSL. Spring Boot support was not addressed on purpose until we have a stable foundation in the core module.

My goal for now is to gather feedback on this (even though I think we already discussed several points and your feedback was invaluable!!). I find the new model more intuitive and easier to think about than the previous one, but I am looking forward to your feedback on that.

I'm not quite sure it's the right place, but maybe I don't fully understand your intentions. I like the Default pattern — it’s similar to, for example, RestClient / WebClient or ChatClient from spring-ai.

I don't see the relationship in design with RestClient and others. What I was trying to say is that I see no need for another class named DefaultCommand next to AbstractCommand as I don't think we can talk about a "default" command? What would a default command do? I think the behaviour of a "base" command is abstract (ie undefined) and should be defined in a concrete command, hence the thinking about AbstractCommand that could be extended to define the actual behaviour of a custom command. We have similar arrangements in other projects from the portfolio like for example AbstractJob and AbstractStep in Spring Batch, as well as AbstractDispatcher in Spring Integration. Here is the new AbstractCommand (we can add a builder for it when we tackle the DSL) which is extended by core commands (help, version, etc) as well as some adapters for functions, consumers, etc in org.springframework.shell.core.commands.adapter.

Hmm, maybe that's a good idea. But are you thinking about defaults for all methods?

Not all methods, but those that have sensible defaults. The goal is to minimize the API surface, and making Command a functional interface is better than having yet another, separate interface just to define command's logic. Again, we have a similar design in Spring Batch like the Step interface that defines both the configuration of the step as well as its execution. Here is an excerpt from the Javadoc of Step:

As with a `Job`, `Step` is meant to explicitly represent the configuration of a step by a developer but also the ability to execute the step.

[piotrooo]

As mentioned previously, the plan is to iterate on this. There are a couple TODOs like options handling, aliases, and command availability. Once that in place, I think we can tackle the new DSL. Spring Boot support was not addressed on purpose until we have a stable foundation in the core module.

If I understand you correctly, you won't start implementing the DSL right now?
Yes, Spring Boot support should be added at the very end.

My goal for now is to gather feedback on this (even though I think we already discussed several points and your feedback was invaluable!!). I find the new model more intuitive and easier to think about than the previous one, but I am looking forward to your feedback on that.

Definitely! The new model is much smoother. However, I didn't notice any @ExitCode or @ExceptionResolver annotation, which I think are really convenient. How about that change? Is there any substitute?

What about parts of the existing features like CommandExitCode? Since now we always need to iterate with the Terminal object? The ability not to do that (or to do it only minimally) was one of Spring Shell's strengths.

Are we intentionally resigning from that?

I don't see the relationship in design with RestClient and others. What I was trying to say is that I see no need for another class named DefaultCommand next to AbstractCommand as I don't think we can talk about a "default" command? What would a default command do?

Very good argument, thanks!

So what are the next steps? Should I do something with the DSL? If so, it could be done this way (or how do you imagine it?):

Command.builder()
    .name("say-hello")
    .description("Command for saing hello")
    .group("greetings")
    .help("This is a help string of say-hello command")
    // Multiple calls
    .withOption(optionSpec -> optionSpec
                    .longName("name")
                    .shortName('s')
                    .description("Say this name")
                    .type(ResolvableType) // see #1216
                    .type(Type)
                    .required(boolean)
                    .required() // default 'true'
                    .defaultValue("John")
                    .position(int)
                    .arity(OptionArity)
                    .arity(int, int)
    )
    // Multiple calls, define multiple aliases (?)
    .withAlias(aliasSpec -> aliasSpec
                    .command("say-hello-alias")
                    .group("greetings")
    )
    .build()

That should be all for the first iteration, right?

[fmbenhassine]

Can you please rebase this on the latest main? I pushed 97121ac to refine the programming model and which now provides the basic building blocs for the DSL. Sub commands, command availability and aliases are not supported yet, but they will come in 4.0 M3. The support for option arity was removed, I will explain the reason in a separate issue.

So in this first iteration of the DSL, please do include only what is currently supported, and we will add other features in next iterations (make it work, then make it better 😉). I target the initial version of the new DSL in the upcoming 4.0 M2.

Many thanks upfront!

[fmbenhassine]

So what are the next steps? Should I do something with the DSL? If so, it could be done this way (or how do you imagine it?):

What you suggest is great! Options and arguments are available through the command context, so no need to define them in the DSL. For aliases (Multiple calls, define multiple aliases (?)) , we can keep it simple with varargs. Here is an example of what the first iteration could support (the simplest possible):

@Bean
public Command cmd() {
    return Command.builder();
            .name("cmd")
            .description("A command defined as a bean")
            .help("This is a sample command defined as a Spring Bean")
            .group("greetings")
            .aliases("alias1", "alias2")
            .execute(ctx -> {
                System.out.println("Executing cmd command");
                return ExitStatus.OK;
            })
            .build();
}

You can leverage the ConsumerCommandAdapter to build the command. Let me know if you need help on this!

[fmbenhassine]

Has this been rebased on the latest main? I saw a force push but it is still using old APIs that were removed in 97121ac.

Please cc me here when you think this is ready to review (with the minimal change set as explained in my previous comments). Many thanks upfront 🙏

[piotrooo]

@fmbenhassine hey Mahmoud 👋

It's not done yet. This week I've had very limited free time, so I only managed to do the rebase yesterday 😭
Sorry about that.

I still need to do some work here. I hope I’ll be able to finish it next week.

[fmbenhassine]

Sure! no problem take your time no rush.

Thank you upfront 🙏

[piotrooo]

@fmbenhassine I think I have it!

One thing I don’t like is this hacking way of creating the command execution logic:

.execute(commandContext -> {
	String ansiString = new AttributedStringBuilder().append("Good morning ")
		.append("Sir", BOLD.foreground(GREEN))
		.append("!")
		.toAnsi();

	Terminal terminal = commandContext.terminal();
	terminal.writer().println(ansiString);
	terminal.flush();
})

I'd prefer to have something like:

.execute(commandContext -> "some string")

Without all this complicated flushing and using the terminal.

But we can address this in the next steps, since it’s not easy to define execution precedence. I mean something like:

.execute(commandContext -> {
	String ansiString = new AttributedStringBuilder().append("Good morning ")
		.append("Sir", BOLD.foreground(GREEN))
		.append("!")
		.toAnsi();

	Terminal terminal = commandContext.terminal();
	terminal.writer().println(ansiString);
	terminal.flush();
})
.execute(commandContext -> "some string")

Which execute() should be executed? But as I said — next steps.

[fmbenhassine]

Great! I will take a look early next week. Just one thing, please remove the Boot related changes from this PR, I will take care of that later.

I'd prefer to have something like:
.execute(commandContext -> "some string")

What you are asking for is already possible with the functional command adapter: https://github.com/spring-projects/spring-shell/blob/main/spring-shell-core/src/main/java/org/springframework/shell/core/commands/adapter/FunctionCommandAdapter.java

That's the idea behind this "adapters" package, we can add more adapters if needed.

Which execute() should be executed? But as I said — next steps.
But we can address this in the next steps, since it’s not easy to define execution precedence. I mean something like:

In the DSL there should be only one call to the execute method (no precedence or ambiguity), and this execute method could/should be overloaded to accept a consumer, a function, etc. With that in place, the user has the choice between the provided options.

[fmbenhassine]

That should be the group, not an alias, right?

Aliases are not supported yet BTW. They are planned for 4.0 M3.

[piotrooo]

In the DSL there should be only one call to the execute method (no precedence or ambiguity), and this execute method could/should be overloaded to accept a consumer, a function, etc. With that in place, the user has the choice between the provided options.

This is what I was also thinking about, so to be clear: if these builder definitions are OK for you:

Builder execute(Consumer<CommandContext> commandExecutor);

Builder execute(Function<CommandContext, String> commandExecutor);

we can implement them right now.

To prevent using the execute() method twice, maybe these methods could return a builder that only has a single build() method.

[fmbenhassine]

This is what I was also thinking about, so to be clear: if these builder definitions are OK for you:

Yes, that would be great!

To prevent using the execute() method twice, maybe these methods could return a builder that only has a single build() method.

Exactly, calling execute would be the last step in the DSL and would return a builder with a single build method that returns the fully defined command.

Looking forward to reviewing this!

[fmbenhassine]

Since we have a method accepting aliases as varargs and to keep the API minimalistic, I think there is no need for another one that accepts a list (conversion is always possible on the client side).

[fmbenhassine]

I don't think the Builder should be an interface with a default implementation (creating a custom user-defined builder is not very likely). There are of course good reasons to make a concept configurable/swappable by defining it in an interface/contract, but for builders I am not sure we need that (at least for the most typical average usage). For example, the RetryPolicy in Spring Framework defines a builder in the interface, and it is a static inner class and not an interface + default implementation: https://github.com/spring-projects/spring-framework/blob/main/spring-core/src/main/java/org/springframework/core/retry/RetryPolicy.java.

So to keep things simple, can you please follow the same approach? Thank you upfront.

[fmbenhassine]

AbstractCommand is a leaky abstraction here (look at the import coming from a different package). The builder should return the interface type (or at least a package private type that we can't break from the outside, but since we don't have that, returning the interface type is the way to go).

[fmbenhassine]

What about using the overloaded method that takes a Function<CommandContext, String> and remove the terminal printing /flushing noise?

That would be a good example to show how to print styled output without dealing with terminal details.

[fmbenhassine]

The code here is used in a single place, so it could be inlined.

[fmbenhassine]

Looking forward to reviewing this!

Thank you for the updates! LGTM now 👍

I added a couple minor suggestions. If you agree, please update the PR accordingly and it should be good to merge. I can also take care of these changes on merge if you want.