cli_parser: Refactor interface for depends, conflicts and add tests

Library/Homebrew/cli_parser.rb

@@ -1,5 +1,6 @@
 require "optparse"
 require "ostruct"
+require "set"
 
 module Homebrew
   module CLI
@@ -13,7 +14,7 @@ module Homebrew
         @parsed_args = OpenStruct.new
         # undefine tap to allow --tap argument
         @parsed_args.instance_eval { undef tap }
-        @depends = []
+        @constraints = []
         @conflicts = []
         instance_eval(&block)
       end
@@ -36,7 +37,7 @@ module Homebrew
         end
       end
 
-      def flag(name, description: nil)
+      def flag(name, description: nil, required_for: nil, depends_on: nil)
         if name.end_with? "="
           required = OptionParser::REQUIRED_ARGUMENT
           name.chomp! "="
@@ -47,18 +48,16 @@ module Homebrew
         @parser.on(name, description, required) do |option_value|
           @parsed_args[option_to_name(name)] = option_value
         end
+
+        set_constraints(name, required_for: required_for, depends_on: depends_on)
       end
 
-      def depends(primary, secondary, mandatory: false)
+      def conflicts(*options)
-        @depends << [primary, secondary, mandatory]
+        @conflicts << options.map { |option| option_to_name(option) }
-      end
-
-      def conflicts(primary, secondary)
-        @conflicts << [primary, secondary]
       end
 
       def option_to_name(name)
-        name.sub(/\A--?/, "").tr("-", "_")
+        name.sub(/\A--?/, "").tr("-", "_").delete("=")
       end
 
       def option_to_description(*names)
@@ -98,32 +97,57 @@ module Homebrew
         @parsed_args.respond_to?(name) || @parsed_args.respond_to?("#{name}?")
       end
 
-      def check_depends
+      def set_constraints(name, depends_on:, required_for:)
-        @depends.each do |primary, secondary, required|
+        secondary = option_to_name(name)
+        unless required_for.nil?
+          primary = option_to_name(required_for)
+          @constraints << [primary, secondary, :mandatory]
+        end
+
+        return if depends_on.nil?
+        primary = option_to_name(depends_on)
+        @constraints << [primary, secondary, :optional]
+      end
+
+      def check_constraints
+        @constraints.each do |primary, secondary, constraint_type|
           primary_passed = option_passed?(primary)
           secondary_passed = option_passed?(secondary)
-          raise OptionDependencyError.new(primary, secondary) if required && primary_passed &&
+          if :mandatory.equal?(constraint_type) && primary_passed && !secondary_passed
-                                                                 !secondary_passed
+            raise OptionConstraintError.new(primary, secondary)
-          raise OptionDependencyError.new(primary, secondary, missing: true) if secondary_passed &&
+          end
-                                                                                !primary_passed
+          if secondary_passed && !primary_passed
+            raise OptionConstraintError.new(primary, secondary, missing: true)
+          end
         end
       end
 
       def check_conflicts
-        @conflicts.each do |primary, secondary|
+        @conflicts.each do |mutually_exclusive_options_group|
-          primary_passed = option_passed?(primary)
+          violations = mutually_exclusive_options_group.select do |option|
-          secondary_passed = option_passed?(secondary)
+            option_passed? option
-          raise OptionConflictError.new(primary, secondary) if primary_passed && secondary_passed
+          end
+          raise OptionConflictError, violations if violations.length > 1
+        end
+      end
+
+      def check_invalid_constraints
+        @conflicts.each do |mutually_exclusive_options_group|
+          @constraints.each do |p, s|
+            next unless Set[p, s].subset?(Set[*mutually_exclusive_options_group])
+            raise InvalidConstraintError.new(p, s)
+          end
         end
       end
 
       def check_constraint_violations
+        check_invalid_constraints
         check_conflicts
-        check_depends
+        check_constraints
       end
     end
 
-    class OptionDependencyError < RuntimeError
+    class OptionConstraintError < RuntimeError
       def initialize(arg1, arg2, missing: false)
         if !missing
           message = <<~EOS
@@ -139,9 +163,18 @@ module Homebrew
     end
 
     class OptionConflictError < RuntimeError
+      def initialize(args)
+        args_list = args.join("` and `")
+        super <<~EOS
+          `#{args_list}` are mutually exclusive
+        EOS
+      end
+    end
+
+    class InvalidConstraintError < RuntimeError
       def initialize(arg1, arg2)
         super <<~EOS
-          `#{arg1}` and `#{arg2}` should not be passed together
+          `#{arg1}` and `#{arg2}` cannot be mutually exclusive and mutually dependent simultaneously
         EOS
       end
     end

Library/Homebrew/dev-cmd/bump-formula-pr.rb

@@ -49,26 +49,26 @@ module Homebrew
 
   def bump_formula_pr
     @args = Homebrew::CLI::Parser.parse do
-      switch "--devel"
+      switch    "--devel"
-      switch "-n", "--dry-run"
+      switch    "-n", "--dry-run"
-      switch "--write"
+      switch    "--write"
-      switch "--audit"
+      switch    "--audit"
-      switch "--strict"
+      switch    "--strict"
-      switch "--no-browse"
+      switch    "--no-browse"
-      switch :quiet
+      switch    :quiet
-      switch :force
+      switch    :force
-      switch :verbose
+      switch    :verbose
-      switch :debug
+      switch    :debug
-      flag   "--url",      required: true
+
-      flag   "--sha256",   required: true
+      flag      "--url="
-      flag   "--mirror",   required: true
+      flag      "--revision="
-      flag   "--tag",      required: true
+      flag      "--tag=",         required_for: "--revision="
-      flag   "--revision", required: true
+      flag      "--sha256=",      depends_on: "--url="
-      flag   "--version",  required: true
+      flag      "--mirror="
-      flag   "--message",  required: true
+      flag      "--version="
-      depends :url, :sha256
+      flag      "--message="
-      depends :tag, :revision, mandatory: true
+
-      conflicts :url, :tag
+      conflicts "--url", "--tag"
     end
 
     # As this command is simplifying user run commands then let's just use a

Library/Homebrew/test/cli_parser_spec.rb

@@ -74,22 +74,21 @@ describe Homebrew::CLI::Parser do
   describe "test constraints" do
     subject(:parser) {
       described_class.new do
-        flag "--flag1"
+        flag      "--flag1"
-        flag "--flag2"
+        flag      "--flag3"
-        flag "--flag3"
+        flag      "--flag2", required_for: "--flag1"
-        flag "--flag4"
+        flag      "--flag4", depends_on: "--flag3"
-        depends :flag1, :flag2, mandatory: true
+
-        depends :flag3, :flag4
+        conflicts "--flag1", "--flag3"
-        conflicts :flag1, :flag3
       end
     }
 
     it "raises exception on depends mandatory constraint violation" do
-      expect { parser.parse(["--flag1"]) }.to raise_error(Homebrew::CLI::OptionDependencyError)
+      expect { parser.parse(["--flag1"]) }.to raise_error(Homebrew::CLI::OptionConstraintError)
     end
 
     it "raises exception on depends constraint violation" do
-      expect { parser.parse(["--flag2"]) }.to raise_error(Homebrew::CLI::OptionDependencyError)
+      expect { parser.parse(["--flag2"]) }.to raise_error(Homebrew::CLI::OptionConstraintError)
     end
 
     it "raises exception for conflict violation" do
@@ -107,4 +106,18 @@ describe Homebrew::CLI::Parser do
       expect(args.flag3).to be true
     end
   end
+
+  describe "test invalid constraints" do
+    subject(:parser) {
+      described_class.new do
+        flag      "--flag1"
+        flag      "--flag2", depends_on: "--flag1"
+        conflicts "--flag1", "--flag2"
+      end
+    }
+
+    it "raises exception due to invalid constraints" do
+      expect { parser.parse([]) }.to raise_error(Homebrew::CLI::InvalidConstraintError)
+    end
+  end
 end