ctubbsii commented on code in PR #6121:
URL: https://github.com/apache/accumulo/pull/6121#discussion_r2790316811
##########
shell/src/main/java/org/apache/accumulo/shell/ShellOptions.java:
##########
Review Comment:
It's confusing to combine the file that represents commons-cli options with
JCommander options. It might be better to keep these separate. If they are
entangled, it may make it difficult to modify CLI options for the shell command
without breaking commands inside the shell, or vice versa.
##########
shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java:
##########
@@ -96,14 +95,15 @@ public String[] args(String... args) {
}
@Test
- public void testHelp() throws IOException {
- assertFalse(shell.config(args("--help")));
+ public void testHelp() throws Exception {
+ shell.execute(args("--help"));
assertTrue(output.get().startsWith("Usage"), "Did not print usage");
}
@Test
- public void testBadArg() throws IOException {
- assertFalse(shell.config(args("--bogus")));
+ public void testBadArg() throws Exception {
+ shell.execute(args("--bogus"));
+ assertTrue(shell.getExitCode() != 0);
Review Comment:
I think `assertNotEquals` is a thing, and it should have a more informative
error message when it fails.
```suggestion
assertNotEquals(0, shell.getExitCode());
```
##########
core/src/main/java/org/apache/accumulo/core/cli/ClientKeywordExecutable.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License 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 org.apache.accumulo.core.cli;
+
+import org.apache.accumulo.start.spi.KeywordExecutable;
+
+import com.beust.jcommander.JCommander;
+import com.beust.jcommander.ParameterException;
+
+public abstract class ClientKeywordExecutable<O extends ClientOpts> implements
KeywordExecutable {
+
+ private final O options;
Review Comment:
This looks like "zero options" 😺
Although it tends to be a convention, it is not strictly necessary to use a
single character generic parameter type name. It could be "OPTS", or even
"OPTIONS" instead. It is only used in a few places in this file, so it doesn't
need to be excessively short.
##########
start/src/main/java/org/apache/accumulo/start/spi/KeywordExecutable.java:
##########
@@ -44,10 +44,6 @@
*/
public interface KeywordExecutable {
- enum UsageGroup {
- ADMIN, COMPACTION, CORE, PROCESS, OTHER
- }
Review Comment:
Moving this breaks the SPI. This should stay in place, so as not to break
existing KeywordExecutable classes that users might have written. The use of a
default method below was an attempt to ensure backward-compatibility, but that
effort is undermined by breaking it this way.
Instead, a new type (maybe "CommandGroup"?) should be added, with a default
method of its own. We can deprecate the usageGroup method and the UsageGroup
enum, if it is no longer needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
