Rework command subsystem #396
Conversation
- Focus of these changes are to introduce a new command system based on real registrations (new way) instead of continuously (old way) resolve methods and its parameters via reflection. - There's a lot of changes as this resolution via reflection had its hooks almost everywhere and thus most changes are just refactorings. - Order to understand real changes I'd start to look classes under `org.springframework.shell.command` package as it defines new registration, catalog and parser classes. Also samples contain new classes to demonstrate new functionality. - Fixes spring-projects#380
There was a problem hiding this comment.
@jvalkeal I have taken a 1st pass but am going to read the docs and then go back through spring-shell-core changes in a 2nd pass. I will submit my current review comments though and continue more tomorrow.
One thing I want to understand more is the ripple this will have in SCDF/Skipper shells.
| return catalog; | ||
| } | ||
|
|
||
| @Bean |
There was a problem hiding this comment.
- I would consider ordering this default customizer as line44 spins through them ordered.
- Would we ever want to disable or override this default behavior?
There was a problem hiding this comment.
- Now that you mentioned it, we probably should have a flag.
| void unregister(String... commandName); | ||
|
|
||
| /** | ||
| * Gets all {@link CommandRegistration}s mapped with their names. |
There was a problem hiding this comment.
I know this is a high level review - but I would eventually elaborate a bit more here as the default impl resolves dynamic commands as well as filter based on interactive/non-interactive.
Also, I think its important to mention the map is a copy and any updates made to it will be ignored.
There was a problem hiding this comment.
This is a bit difficult design part of this interface. As you already commented an actual command name is and its use is spanning CommandCatalog and CommandRegistration. I originally had this just returning a list of registrations but then you need to calculate names out from registrations which felt weird.
| * some commands may or may not exist depending on a runtime state. | ||
| */ | ||
| @FunctionalInterface | ||
| interface CommandResolver { |
There was a problem hiding this comment.
Any reason to put these interfaces in here nested? I have found that nesting the interfaces in designs makes it harder to resonate with.
I would strongly prefer the default impl to be outside of the interface definition as well.
There was a problem hiding this comment.
Indeed, I've had same thoughts.
| */ | ||
| static class DefaultCommandCatalog implements CommandCatalog { | ||
|
|
||
| private final HashMap<String, CommandRegistration> commandRegistrations = new HashMap<>(); |
There was a problem hiding this comment.
Any particular reason to type to HashMap rather than Map?
| static class DefaultCommandCatalog implements CommandCatalog { | ||
|
|
||
| private final HashMap<String, CommandRegistration> commandRegistrations = new HashMap<>(); | ||
| private final Collection<CommandResolver> resolvers = new ArrayList<>(); |
There was a problem hiding this comment.
I am curious on the choice of Collection rather than Set or List here.
| .collect(Collectors.toMap(Entry::getKey, Entry::getValue)); | ||
| } | ||
|
|
||
| private static String commandName(String[] commands) { |
There was a problem hiding this comment.
I think it would be an improvement to encapsulate this into a method in CommandRegistration. The Builder talks about this detail and it would be great if that behavior was w/ the CR itself.
* Define commands this registration uses. Essentially defines a full set of
* main and sub commands. It doesn't matter if full command is defined in one
* string or multiple strings as "words" are splitted and trimmed with
* whitespaces. You will get result of {@code command subcommand1 subcommand2, ...}.There was a problem hiding this comment.
Yes, and I think I'll polish interfaces around this.
| } | ||
| return regs.entrySet().stream() | ||
| .filter(e -> { | ||
| InteractionMode mim = e.getValue().getInteractionMode(); |
There was a problem hiding this comment.
At 1st glance, this hurts my head. I get it now, but it took too long. I have found in "big" filters like this creating a well-named private method and filtering by method reference makes it more readable.
| @@ -45,15 +45,15 @@ public class LineReaderAutoConfiguration { | |||
|
|
|||
| private Parser parser; | |||
|
|
|||
| private CommandRegistry commandRegistry; | |||
| private CommandCatalog commandRegistry; | |||
There was a problem hiding this comment.
I'll mention this once here as its a [NIT] but worth pointing out... the var name needs to change as well. Its still reads "Registry" below in the code.
| ObjectProvider<ValueProvider> valueProviders) { | ||
| Set<ValueProvider> collect = valueProviders.orderedStream().collect(Collectors.toSet()); | ||
| return new StandardParameterResolver(conversionService, collect); | ||
| public CompletionResolver defaultCompletionResolver() { |
There was a problem hiding this comment.
Any need to disable or override this default?
There was a problem hiding this comment.
Currently I don't think so.
| .interactionMode(shellMapping.interactionMode()) | ||
| .availability(availabilityIndicator); | ||
|
|
||
| InvocableHandlerMethod ihm = new InvocableHandlerMethod(bean, method); |
There was a problem hiding this comment.
Can you leave a comment here describing what all this is doing? I got lost on this one. :)
|
I'm going to merge this. Think I've addresses most of a comments and reworked this pr. I'll create follow-up stories for some things which needs a bit more thinking. |
|
Merged per 8a23518 |
real registrations (new way) instead of continuously (old way) resolve
methods and its parameters via reflection.
hooks almost everywhere and thus most changes are just refactorings.
org.springframework.shell.commandpackage as it defines new registration,catalog and parser classes. Also samples contain new classes to demonstrate
new functionality.