gerrit approve: Cleanup option parsing to reduce unnecessary code
A lot of this code is just unnecessary complexity, instead we can pass through the ApprovalType and save quite a few lines of code. Change-Id: I02f51beeb35bf9e464c243bd0aa63336d9646dce Signed-off-by: Shawn O. Pearce <sop@google.com>
This commit is contained in:
		@@ -49,7 +49,7 @@ public class ApproveCommand extends BaseCommand {
 | 
			
		||||
  @Override
 | 
			
		||||
  protected final CmdLineParser newCmdLineParser() {
 | 
			
		||||
    final CmdLineParser parser = super.newCmdLineParser();
 | 
			
		||||
    for (CmdOption c : optionList) {
 | 
			
		||||
    for (ApproveOption c : optionList) {
 | 
			
		||||
      parser.addOption(c, c);
 | 
			
		||||
    }
 | 
			
		||||
    return parser;
 | 
			
		||||
@@ -82,14 +82,14 @@ public class ApproveCommand extends BaseCommand {
 | 
			
		||||
  @Inject
 | 
			
		||||
  private FunctionState.Factory functionStateFactory;
 | 
			
		||||
 | 
			
		||||
  private List<CmdOption> optionList;
 | 
			
		||||
  private List<ApproveOption> optionList;
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final void start() {
 | 
			
		||||
    startThread(new CommandRunnable() {
 | 
			
		||||
      @Override
 | 
			
		||||
      public void run() throws Exception {
 | 
			
		||||
        getApprovalNames();
 | 
			
		||||
        initOptionList();
 | 
			
		||||
        parseCommandLine();
 | 
			
		||||
 | 
			
		||||
        final Transaction txn = db.beginTransaction();
 | 
			
		||||
@@ -113,9 +113,8 @@ public class ApproveCommand extends BaseCommand {
 | 
			
		||||
        sb.append(patchSetId.get());
 | 
			
		||||
        sb.append(": ");
 | 
			
		||||
 | 
			
		||||
        for (CmdOption co : optionList) {
 | 
			
		||||
          ApprovalCategory.Id category =
 | 
			
		||||
              new ApprovalCategory.Id(co.approvalKey());
 | 
			
		||||
        for (ApproveOption co : optionList) {
 | 
			
		||||
          final ApprovalCategory.Id category = co.getCategoryId();
 | 
			
		||||
          PatchSetApproval.Key psaKey =
 | 
			
		||||
              new PatchSetApproval.Key(patchSetId, currentUser.getAccountId(),
 | 
			
		||||
                  category);
 | 
			
		||||
@@ -175,7 +174,7 @@ public class ApproveCommand extends BaseCommand {
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  private void addApproval(final PatchSetApproval.Key psaKey,
 | 
			
		||||
      final Short score, final Change c, final CmdOption co,
 | 
			
		||||
      final Short score, final Change c, final ApproveOption co,
 | 
			
		||||
      final Transaction txn) throws OrmException, UnloggedFailure {
 | 
			
		||||
    PatchSetApproval psa = db.patchSetApprovals().get(psaKey);
 | 
			
		||||
    boolean insert = false;
 | 
			
		||||
@@ -203,8 +202,8 @@ public class ApproveCommand extends BaseCommand {
 | 
			
		||||
    }
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  private void getApprovalNames() {
 | 
			
		||||
    optionList = new ArrayList<CmdOption>();
 | 
			
		||||
  private void initOptionList() {
 | 
			
		||||
    optionList = new ArrayList<ApproveOption>();
 | 
			
		||||
 | 
			
		||||
    for (ApprovalType type : approvalTypes.getApprovalTypes()) {
 | 
			
		||||
      String usage = "";
 | 
			
		||||
@@ -213,14 +212,13 @@ public class ApproveCommand extends BaseCommand {
 | 
			
		||||
 | 
			
		||||
      for (ApprovalCategoryValue v : type.getValues()) {
 | 
			
		||||
        usage +=
 | 
			
		||||
            String.format("%3s", CmdOption.format(v.getValue())) + ": "
 | 
			
		||||
            String.format("%3s", ApproveOption.format(v.getValue())) + ": "
 | 
			
		||||
                + v.getName() + "\n";
 | 
			
		||||
      }
 | 
			
		||||
 | 
			
		||||
      optionList.add(new CmdOption("--"
 | 
			
		||||
          + category.getName().toLowerCase().replace(' ', '-'), usage, category
 | 
			
		||||
          .getId().get(), type.getMin().getValue(), type.getMax().getValue(),
 | 
			
		||||
          category.getName()));
 | 
			
		||||
      final String name =
 | 
			
		||||
          "--" + category.getName().toLowerCase().replace(' ', '-');
 | 
			
		||||
      optionList.add(new ApproveOption(name, usage, type));
 | 
			
		||||
    }
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
 
 | 
			
		||||
@@ -14,6 +14,9 @@
 | 
			
		||||
 | 
			
		||||
package com.google.gerrit.server.ssh.commands;
 | 
			
		||||
 | 
			
		||||
import com.google.gerrit.client.data.ApprovalType;
 | 
			
		||||
import com.google.gerrit.client.reviewdb.ApprovalCategory;
 | 
			
		||||
 | 
			
		||||
import org.kohsuke.args4j.CmdLineException;
 | 
			
		||||
import org.kohsuke.args4j.CmdLineParser;
 | 
			
		||||
import org.kohsuke.args4j.Option;
 | 
			
		||||
@@ -24,91 +27,58 @@ import org.kohsuke.args4j.spi.Setter;
 | 
			
		||||
 | 
			
		||||
import java.lang.annotation.Annotation;
 | 
			
		||||
 | 
			
		||||
class CmdOption implements Option, Setter<Short> {
 | 
			
		||||
  private String metaVar;
 | 
			
		||||
  private boolean multiValued;
 | 
			
		||||
  private String name;
 | 
			
		||||
  private boolean required;
 | 
			
		||||
  private String usage;
 | 
			
		||||
 | 
			
		||||
  private String approvalKey;
 | 
			
		||||
  private Short approvalMax;
 | 
			
		||||
  private Short approvalMin;
 | 
			
		||||
  private String descrName;
 | 
			
		||||
final class ApproveOption implements Option, Setter<Short> {
 | 
			
		||||
  private final String name;
 | 
			
		||||
  private final String usage;
 | 
			
		||||
  private final ApprovalType type;
 | 
			
		||||
 | 
			
		||||
  private Short value;
 | 
			
		||||
 | 
			
		||||
  public CmdOption(final String name, final String usage, final String key,
 | 
			
		||||
      final Short min, final Short max, final String descrName) {
 | 
			
		||||
  ApproveOption(final String name, final String usage, final ApprovalType type) {
 | 
			
		||||
    this.name = name;
 | 
			
		||||
    this.usage = usage;
 | 
			
		||||
 | 
			
		||||
    this.metaVar = "";
 | 
			
		||||
    this.multiValued = false;
 | 
			
		||||
    this.required = false;
 | 
			
		||||
    this.value = null;
 | 
			
		||||
 | 
			
		||||
    this.approvalKey = key;
 | 
			
		||||
    this.approvalMax = max;
 | 
			
		||||
    this.approvalMin = min;
 | 
			
		||||
    this.descrName = descrName;
 | 
			
		||||
    this.type = type;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final String[] aliases() {
 | 
			
		||||
  public String[] aliases() {
 | 
			
		||||
    return new String[0];
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final Class<? extends OptionHandler<Short>> handler() {
 | 
			
		||||
  public Class<? extends OptionHandler<Short>> handler() {
 | 
			
		||||
    return Handler.class;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final String metaVar() {
 | 
			
		||||
    return metaVar;
 | 
			
		||||
  public String metaVar() {
 | 
			
		||||
    return "N";
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final boolean multiValued() {
 | 
			
		||||
    return multiValued;
 | 
			
		||||
  public boolean multiValued() {
 | 
			
		||||
    return false;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final String name() {
 | 
			
		||||
  public String name() {
 | 
			
		||||
    return name;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final boolean required() {
 | 
			
		||||
    return required;
 | 
			
		||||
  public boolean required() {
 | 
			
		||||
    return false;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public final String usage() {
 | 
			
		||||
  public String usage() {
 | 
			
		||||
    return usage;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  public final Short value() {
 | 
			
		||||
  public Short value() {
 | 
			
		||||
    return value;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  public final String approvalKey() {
 | 
			
		||||
    return approvalKey;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  public final Short approvalMax() {
 | 
			
		||||
    return approvalMax;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  public final Short approvalMin() {
 | 
			
		||||
    return approvalMin;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  public final String descrName() {
 | 
			
		||||
    return descrName;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
  public Class<? extends Annotation> annotationType() {
 | 
			
		||||
    return null;
 | 
			
		||||
@@ -129,13 +99,17 @@ class CmdOption implements Option, Setter<Short> {
 | 
			
		||||
    return false;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  ApprovalCategory.Id getCategoryId() {
 | 
			
		||||
    return type.getCategory().getId();
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  public static class Handler extends OneArgumentOptionHandler<Short> {
 | 
			
		||||
    private final CmdOption cmdOption;
 | 
			
		||||
    private final ApproveOption cmdOption;
 | 
			
		||||
 | 
			
		||||
    public Handler(final CmdLineParser parser, final OptionDef option,
 | 
			
		||||
        final Setter<Short> setter) {
 | 
			
		||||
      super(parser, option, setter);
 | 
			
		||||
      this.cmdOption = (CmdOption) setter;
 | 
			
		||||
      this.cmdOption = (ApproveOption) setter;
 | 
			
		||||
    }
 | 
			
		||||
 | 
			
		||||
    @Override
 | 
			
		||||
@@ -147,8 +121,8 @@ class CmdOption implements Option, Setter<Short> {
 | 
			
		||||
      }
 | 
			
		||||
 | 
			
		||||
      final short value = Short.parseShort(argument);
 | 
			
		||||
      final short min = cmdOption.approvalMin;
 | 
			
		||||
      final short max = cmdOption.approvalMax;
 | 
			
		||||
      final short min = cmdOption.type.getMin().getValue();
 | 
			
		||||
      final short max = cmdOption.type.getMax().getValue();
 | 
			
		||||
 | 
			
		||||
      if (value < min || value > max) {
 | 
			
		||||
        final String name = cmdOption.name();
 | 
			
		||||
		Reference in New Issue
	
	Block a user