From 07ee23d711954bfada003d9edf1fa7977a00e682 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Sat, 14 Apr 2018 16:17:14 +0530 Subject: [PATCH] cli_parser: Refactor interface for depends, conflicts and add tests --- Library/Homebrew/cli_parser.rb | 77 +++++++++++++++------ Library/Homebrew/dev-cmd/bump-formula-pr.rb | 40 +++++------ Library/Homebrew/test/cli_parser_spec.rb | 31 ++++++--- 3 files changed, 97 insertions(+), 51 deletions(-) diff --git a/Library/Homebrew/cli_parser.rb b/Library/Homebrew/cli_parser.rb index d92a2a4ff4..228eab7dd9 100644 --- a/Library/Homebrew/cli_parser.rb +++ b/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) - @depends << [primary, secondary, mandatory] - end - - def conflicts(primary, secondary) - @conflicts << [primary, secondary] + def conflicts(*options) + @conflicts << options.map { |option| option_to_name(option) } 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 - @depends.each do |primary, secondary, required| + def set_constraints(name, depends_on:, required_for:) + 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 && - !secondary_passed - raise OptionDependencyError.new(primary, secondary, missing: true) if secondary_passed && - !primary_passed + if :mandatory.equal?(constraint_type) && primary_passed && !secondary_passed + raise OptionConstraintError.new(primary, secondary) + end + if secondary_passed && !primary_passed + raise OptionConstraintError.new(primary, secondary, missing: true) + end end end def check_conflicts - @conflicts.each do |primary, secondary| - primary_passed = option_passed?(primary) - secondary_passed = option_passed?(secondary) - raise OptionConflictError.new(primary, secondary) if primary_passed && secondary_passed + @conflicts.each do |mutually_exclusive_options_group| + violations = mutually_exclusive_options_group.select do |option| + option_passed? option + 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 diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index 8a3abba080..66a72fe2a4 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/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 "-n", "--dry-run" - switch "--write" - switch "--audit" - switch "--strict" - switch "--no-browse" - switch :quiet - switch :force - switch :verbose - switch :debug - flag "--url", required: true - flag "--sha256", required: true - flag "--mirror", required: true - flag "--tag", required: true - flag "--revision", required: true - flag "--version", required: true - flag "--message", required: true - depends :url, :sha256 - depends :tag, :revision, mandatory: true - conflicts :url, :tag + switch "--devel" + switch "-n", "--dry-run" + switch "--write" + switch "--audit" + switch "--strict" + switch "--no-browse" + switch :quiet + switch :force + switch :verbose + switch :debug + + flag "--url=" + flag "--revision=" + flag "--tag=", required_for: "--revision=" + flag "--sha256=", depends_on: "--url=" + flag "--mirror=" + flag "--version=" + flag "--message=" + + conflicts "--url", "--tag" end # As this command is simplifying user run commands then let's just use a diff --git a/Library/Homebrew/test/cli_parser_spec.rb b/Library/Homebrew/test/cli_parser_spec.rb index 82ab944363..418d3234fa 100644 --- a/Library/Homebrew/test/cli_parser_spec.rb +++ b/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 "--flag2" - flag "--flag3" - flag "--flag4" - depends :flag1, :flag2, mandatory: true - depends :flag3, :flag4 - conflicts :flag1, :flag3 + flag "--flag1" + flag "--flag3" + flag "--flag2", required_for: "--flag1" + flag "--flag4", depends_on: "--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