[
https://issues.apache.org/jira/browse/GEODE-3007?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16248200#comment-16248200
]
ASF GitHub Bot commented on GEODE-3007:
---------------------------------------
PurelyApplied commented on a change in pull request #1042: GEODE-3007: Simplify
support for custom GFSH commands
URL: https://github.com/apache/geode/pull/1042#discussion_r150362381
##########
File path:
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
##########
@@ -57,10 +57,14 @@ public GfshParser(CommandManager commandManager) {
add(command);
}
- for (Converter<?> converter : commandManager.getConverters()) {
+ for (Converter converter : commandManager.getConverters()) {
if (converter.getClass().isAssignableFrom(ArrayConverter.class)) {
ArrayConverter arrayConverter = (ArrayConverter) converter;
- arrayConverter.setConverters(new
HashSet<>(commandManager.getConverters()));
+ HashSet<Converter<?>> converters = new HashSet<>();
+ for (Converter c : commandManager.getConverters()) {
+ converters.add(c);
+ }
+ arrayConverter.setConverters(converters);
Review comment:
At the very least, that should be a `Set` and not a `HashSet` on the left.
If we wanted to collapse this into a `stream`, we could.
```
Set<Converter<?>> converters = new HashSet<>();
for (Converter c : commandManager.getConverters()) {
converters.add(c);
}
```
It seems to me that this is just doing some strange workaround to get a set
of `Converter` to a set of `Converter<?>`. That could be rewritten as
```
Set<Converter<?>> converters =
commandManager.getConverters().stream().map(c -> (Converter<?>) c).collect(
Collectors.toSet());
```
But at that point, the whole loop would go from
```
for (Converter converter : commandManager.getConverters()) {
if (converter.getClass().isAssignableFrom(ArrayConverter.class)) {
ArrayConverter arrayConverter = (ArrayConverter) converter;
Set<Converter<?>> converters =
commandManager.getConverters().stream().map(c -> (Converter<?>) c).collect(
Collectors.toSet());
arrayConverter.setConverters(converters);
}
add(converter);
}
```
to
```
commandManager.getConverters()
.stream()
.filter(converter ->
converter.getClass().isAssignableFrom(ArrayConverter.class))
.forEach(converter ->
((ArrayConverter) converter).setConverters(
commandManager.getConverters().stream().map(
c -> (Converter<?>) c).collect(
Collectors.toSet())));
commandManager.getConverters().forEach(this::add);
```
with the caveat that the `add(converter)` is now happening for each after
the others have been addressed, and not in the middle of the loop. But I don't
think that should really matter, should it..?
Looking at that, I don't really believe it to be more readable than the
first code block in this comment. So probably split the difference, with
```
for (Converter converter : commandManager.getConverters()) {
if (converter.getClass().isAssignableFrom(ArrayConverter.class)) {
ArrayConverter arrayConverter = (ArrayConverter) converter;
arrayConverter.setConverters(
commandManager.getConverters().stream()
.map(c -> (Converter<?>) c).collect(Collectors.toSet()));
}
add(converter);
}
```
I don't know. I guess you could one step farther and reduce to
```
for (Converter converter : commandManager.getConverters()) {
if (converter.getClass().isAssignableFrom(ArrayConverter.class)) {
((ArrayConverter) converter).setConverters(
commandManager.getConverters().stream()
.map(c -> (Converter<?>) c).collect(Collectors.toSet()));
}
add(converter);
}
```
but I personally don't like one-lining casts and method calls.
...
This comment got a lot bigger than I originally intended...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Simplify support for custom GFSH commands
> -----------------------------------------
>
> Key: GEODE-3007
> URL: https://issues.apache.org/jira/browse/GEODE-3007
> Project: Geode
> Issue Type: Improvement
> Components: docs, gfsh
> Reporter: Jared Stewart
> Assignee: Jared Stewart
>
> Geode currently supports three ways to load GFSH commands:
> 1. Scan the classpath for commands in
> "org.apache.geode.management.internal.cli.commands”
> 2. Scan the classpath for commands in a package specified by a user via the
> “user-command-packages” system property.
> 3. Scan the classpath for commands registered in files inside
> META-INF.services (e.g.
> "geode-core/src/test/resources/META-INF/services/org.springframework.shell.core.CommandMarker”)
>
> After the improvements made by GEODE-2989, there is no reason to require a
> user to specify the location of their custom commands via one of these
> mechanisms. Instead, we should simply scan the entire classpath for any
> classes implementing CommandMarker (regardless of whatever packages they live
> in).
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
