ctubbsii commented on code in PR #3905:
URL: https://github.com/apache/accumulo/pull/3905#discussion_r1391487950
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java:
##########
@@ -70,6 +70,8 @@ public class ScanCommand extends Command {
private Option contextOpt;
private Option executionHintsOpt;
private Option scanServerOpt;
+ protected Option showFewOpt;
+ private Option formatterOpt;
Review Comment:
I think this `formatterOpt` can stay in the list above. Only `showFewOpt`
needed to be moved out to its own line, to make it `protected`.
Ultimately, we shouldn't have comma-separated lists of variables like these
at all, so eventually, it'd be better if they were all in their own separate
lines. But, to keep this PR minimal, it'd be good to move it back.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/GrepCommand.java:
##########
@@ -49,6 +49,20 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
if (cl.getArgList().isEmpty()) {
throw new MissingArgumentException("No terms specified");
}
+ // Configure formatting options
+ final FormatterConfig config = new FormatterConfig();
+ config.setPrintTimestamps(cl.hasOption(timestampOpt.getOpt()));
+ if (cl.hasOption(showFewOpt.getOpt())) {
+ final String showLength = cl.getOptionValue(showFewOpt.getOpt());
Review Comment:
:+1: for fixing this for the grep commands.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java:
##########
@@ -447,6 +448,10 @@ public Options getOptions() {
executionHintsOpt = new Option(null, "execution-hints", true, "Execution
hints map");
scanServerOpt =
new Option("cl", "consistency-level", true, "set consistency level
(experimental)");
+ // Options specific to ScanCommand
+ showFewOpt = new Option("f", "show-few", true, "show only a specified
number of characters");
+ formatterOpt =
+ new Option("fm", "formatter", true, "fully qualified name of the
formatter class to use");
Review Comment:
I don't think these are needed, since they are already defined above on line
435.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/GrepCommand.java:
##########
@@ -123,6 +136,7 @@ public Options getOptions() {
negateOpt = new Option("v", "negate", false, "only include rows without
search term");
opts.addOption(numThreadsOpt);
opts.addOption(negateOpt);
+ opts.addOption(showFewOpt);
Review Comment:
I don't think this is needed, since it's done in the parent class,
ScanCommand.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/GrepCommand.java:
##########
@@ -49,6 +49,20 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
if (cl.getArgList().isEmpty()) {
throw new MissingArgumentException("No terms specified");
}
+ // Configure formatting options
+ final FormatterConfig config = new FormatterConfig();
+ config.setPrintTimestamps(cl.hasOption(timestampOpt.getOpt()));
+ if (cl.hasOption(showFewOpt.getOpt())) {
+ final String showLength = cl.getOptionValue(showFewOpt.getOpt());
+ try {
+ final int length = Integer.parseInt(showLength);
+ config.setShownLength(length);
+ } catch (NumberFormatException nfe) {
+ Shell.log.error("Arg must be an integer.", nfe);
+ } catch (IllegalArgumentException iae) {
+ Shell.log.error("Arg must be greater than one.", iae);
Review Comment:
This error handling logic copied from ScanCommand does not look correct. The
IllegalArgumentException is thrown when the number is less 0, not less than 2,
which this message implies. It should probably just grab `iae.getMessage()` to
show here, so it is more correct. That should be fixed in ScanCommand as well.
This doesn't have to be part of this PR, though. It can be fixed in a
subsequent change, as it's not related to what you're trying to fix here. It's
up to you if you want to make a separate PR to fix that or fix it in this PR.
--
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]
