ctubbsii commented on code in PR #3684:
URL: https://github.com/apache/accumulo/pull/3684#discussion_r1291603450
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -129,8 +131,7 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
// check for deprecation
var theProp = Property.getPropertyByKey(property);
if (theProp != null && theProp.isDeprecated()) {
- if (!forceSet(shellState, cl,
- "Trying to set deprecated property `" + property + "` continue")) {
+ if (!shellState.yorn("Trying to set deprecated property `" + property
+ "` continue")) {
Review Comment:
The old method handled the force option. This change only does the prompt.
You've got to do something like: `!isForceSet() && !shellstate.yorn(...)`. You
could keep the existing `forceSet()` method, but change it's behavior to let
the prompt happen on `shellState` instead of inside that method.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/DropUserCommand.java:
##########
@@ -51,9 +51,7 @@ private void doDropUser(final Shell shellState, final String
user, final boolean
boolean operate = true;
if (!force) {
- shellState.getWriter().flush();
- String line = shellState.getReader().readLine(getName() + " { " + user +
" } (yes|no)? ");
- operate = line != null && (line.equalsIgnoreCase("y") ||
line.equalsIgnoreCase("yes"));
+ operate = shellState.yorn(getName() + " { " + user + " }");
}
if (operate) {
Review Comment:
Same opportunity to simplify the expression without the need for the
intermediate `operate` variable.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -53,13 +53,7 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
size =
ConfigurationTypeHelper.getFixedMemoryAsBytes(cl.getOptionValue(sizeOpt.getOpt()));
}
if (startRow == null && endRow == null && size < 0 && !all) {
- shellState.getWriter().flush();
- String line = shellState.getReader()
- .readLine("Merge the entire table { " + tableName + " } into one
tablet (yes|no)? ");
- if (line == null) {
- return 0;
- }
- if (!line.equalsIgnoreCase("y") && !line.equalsIgnoreCase("yes")) {
+ if (!shellState.yorn("Merge the entire table { " + tableName + " } into
one tablet")) {
Review Comment:
Not related to your PR, but until no-chop merges are complete, I wonder if
this prompt should convey a bit more panic, as in:
`"Are you *REALLY* sure you want to merge the entire table { " + tableName +
" } into one tablet?!?!?!"`
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -117,7 +118,8 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
}
} else if (cl.hasOption(setOpt.getOpt())) {
// set property on table
- String property = cl.getOptionValue(setOpt.getOpt()), value = null;
+ String property = cl.getOptionValue(setOpt.getOpt());
+ String value;
Review Comment:
Thank you for that!
We should sweep through our code in a separate PR and fix all those
multi-assignments (except the ones in try-with-resources or for loop
initialization, of course).
##########
shell/src/main/java/org/apache/accumulo/shell/commands/DeleteNamespaceCommand.java:
##########
@@ -38,17 +38,14 @@ public class DeleteNamespaceCommand extends Command {
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState)
throws Exception {
boolean force = false;
- boolean operate = true;
if (cl.hasOption(forceOpt.getOpt())) {
force = true;
}
String namespace = cl.getArgs()[0];
+ boolean operate = true;
if (!force) {
- shellState.getWriter().flush();
- String line =
- shellState.getReader().readLine(getName() + " { " + namespace + " }
(yes|no)? ");
- operate = line != null && (line.equalsIgnoreCase("y") ||
line.equalsIgnoreCase("yes"));
+ operate = shellState.yorn(getName() + " { " + namespace + " }");
}
if (operate) {
Review Comment:
This can be simplified to `if (force || shellState.yorn(...)) {`
##########
shell/src/main/java/org/apache/accumulo/shell/Shell.java:
##########
@@ -1263,4 +1263,20 @@ public boolean hasExited() {
return exit;
}
+ /**
+ * Prompt user for yes/no using the shell prompt.
+ *
+ * @param prompt the string printed to user, with (yes|no)? appended as the
prompt.
+ * @return true if user enters y | yes.
+ */
+ public boolean yorn(final String prompt) {
Review Comment:
"yorn" is a weird name. "yesno" is more common for this sort of thing.
Here's [an example in
Lua](https://dev.fandom.com/wiki/Global_Lua_Modules/Yesno). "confirm" is also a
good option.
--
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]
