From 15947305565410be51ce32f2224fea04ba00b283 Mon Sep 17 00:00:00 2001 From: Max Meyer Date: Tue, 12 May 2015 19:25:08 +0200 Subject: [PATCH 1/4] Add validator for string, numeric, array and hash --- lib/thor/parser/argument.rb | 38 ++++++++++++++++++++++------ lib/thor/parser/arguments.rb | 29 ++++++++++++++++++--- lib/thor/parser/option.rb | 11 ++++++++ spec/parser/argument_spec.rb | 18 +++++++++++++ spec/parser/option_spec.rb | 24 ++++++++++++++++++ spec/parser/options_spec.rb | 49 +++++++++++++++++++++++++++++++++++- 6 files changed, 157 insertions(+), 12 deletions(-) diff --git a/lib/thor/parser/argument.rb b/lib/thor/parser/argument.rb index 96f0eff78..073d56a52 100644 --- a/lib/thor/parser/argument.rb +++ b/lib/thor/parser/argument.rb @@ -2,7 +2,7 @@ class Thor class Argument #:nodoc: VALID_TYPES = [:numeric, :hash, :array, :string] - attr_reader :name, :description, :enum, :required, :type, :default, :banner + attr_reader :name, :description, :enum, :required, :type, :default, :banner, :validator, :validator_description alias_method :human_name, :name def initialize(name, options = {}) @@ -13,13 +13,15 @@ def initialize(name, options = {}) fail ArgumentError, "#{class_name} name can't be nil." if name.nil? fail ArgumentError, "Type :#{type} is not valid for #{class_name.downcase}s." if type && !valid_type?(type) - @name = name.to_s - @description = options[:desc] - @required = options.key?(:required) ? options[:required] : true - @type = (type || :string).to_sym - @default = options[:default] - @banner = options[:banner] || default_banner - @enum = options[:enum] + @name = name.to_s + @description = options[:desc] + @required = options.key?(:required) ? options[:required] : true + @type = (type || :string).to_sym + @default = options[:default] + @banner = options[:banner] || default_banner + @enum = options[:enum] + @validator = options[:validator] + @validator_description = options[:validator_desc] validate! # Trigger specific validations end @@ -49,6 +51,26 @@ def validate! elsif @enum && !@enum.is_a?(Array) fail ArgumentError, "An argument cannot have an enum other than an array." end + + validate_validator! + end + + def validate_validator! + fail ArgumentError, "A validator needs to respond to #call" if validator_with_invalid_api? + fail ArgumentError, "A validator needs a description. Please define :validator_desc" if validator_without_description? + fail ArgumentError, "It does not make sense to use both :validator and :enum. Please use either :validator or :enum" if use_enum_and_validator_together? + end + + def use_enum_and_validator_together? + validator && enum + end + + def validator_with_invalid_api? + validator && !validator.respond_to?(:call) + end + + def validator_without_description? + validator && !validator_description end def valid_type?(type) diff --git a/lib/thor/parser/arguments.rb b/lib/thor/parser/arguments.rb index 558e763b2..4a351c77e 100644 --- a/lib/thor/parser/arguments.rb +++ b/lib/thor/parser/arguments.rb @@ -99,6 +99,13 @@ def parse_hash(name) while current_is_value? && peek.include?(":") key, value = shift.split(":", 2) + + if @switches.is_a?(Hash) && switch = @switches[name] + if switch.validator && !switch.validator.call(key, value) + fail MalformattedArgumentError, "Expected '#{name}=#{key}:#{value}' to return true for `:validator`, but it returns false" + end + end + fail MalformattedArgumentError, "You can't specify '#{key}' more than once in option '#{name}'; got #{key}:#{hash[key]} and #{key}:#{value}" if hash.include? key hash[key] = value end @@ -117,7 +124,17 @@ def parse_hash(name) def parse_array(name) return shift if peek.is_a?(Array) array = [] - array << shift while current_is_value? + while current_is_value? + value = shift + + if @switches.is_a?(Hash) && switch = @switches[name] + if switch.validator && !switch.validator.call(value) + fail MalformattedArgumentError, "Expected '#{name}=[#{value}, ...]' to return true for `:validator`, but it returns false" + end + end + + array << value + end array end @@ -133,11 +150,15 @@ def parse_numeric(name) end value = $&.index(".") ? shift.to_f : shift.to_i + if @switches.is_a?(Hash) && switch = @switches[name] - if switch.enum && !switch.enum.include?(value) + if switch.validator && !switch.validator.call(value) + fail MalformattedArgumentError, "Expected '#{name}=#{value}' to return true for `:validator`, but it returns false" + elsif switch.enum && !switch.enum.include?(value) fail MalformattedArgumentError, "Expected '#{name}' to be one of #{switch.enum.join(', ')}; got #{value}" end end + value end @@ -152,7 +173,9 @@ def parse_string(name) else value = shift if @switches.is_a?(Hash) && switch = @switches[name] # rubocop:disable AssignmentInCondition - if switch.enum && !switch.enum.include?(value) + if switch.validator && !switch.validator.call(value) + fail MalformattedArgumentError, "Expected '#{name}=#{value}' to return true for `:validator`, but it returns false" + elsif switch.enum && !switch.enum.include?(value) fail MalformattedArgumentError, "Expected '#{name}' to be one of #{switch.enum.join(', ')}; got #{value}" end end diff --git a/lib/thor/parser/option.rb b/lib/thor/parser/option.rb index 8d4e9d444..eb98e093b 100644 --- a/lib/thor/parser/option.rb +++ b/lib/thor/parser/option.rb @@ -109,6 +109,13 @@ def #{type}? def validate! fail ArgumentError, "An option cannot be boolean and required." if boolean? && required? validate_default_type! + validate_validator! + end + + def validate_validator! + fail ArgumentError, "A validator is only supported for type = :string, :numeric, :array or :hash" if type_cannot_have_validator? + + super end def validate_default_type! @@ -125,6 +132,10 @@ def validate_default_type! fail ArgumentError, "An option's default must match its type." unless default_type == @type end + def type_cannot_have_validator? + validator && ![:string, :numeric, :array, :hash].include?(type) + end + def dasherized? name.index("-") == 0 end diff --git a/spec/parser/argument_spec.rb b/spec/parser/argument_spec.rb index 0b1c1cc46..785bf82ca 100644 --- a/spec/parser/argument_spec.rb +++ b/spec/parser/argument_spec.rb @@ -31,6 +31,24 @@ def argument(name, options = {}) argument(:command, :type => :string, :enum => "bar") end.to raise_error(ArgumentError, "An argument cannot have an enum other than an array.") end + + it "raises an error if validator does not have call-method" do + expect do + argument(:command, :type => :string, :validator => Class.new.new, :validator_desc => 'Validator Description') + end.to raise_error(ArgumentError, "A validator needs to respond to #call") + end + + it "raises an error if validator does not have a description" do + expect do + argument(:command, :type => :string, :validator => proc {}) + end.to raise_error(ArgumentError, "A validator needs a description. Please define :validator_desc") + end + + it "raises an error if validator and enum-option are used together" do + expect do + argument(:command, :type => :string, :validator => proc {}, :validator_desc => 'A validator description', :enum => ['a', 'b']) + end.to raise_error(ArgumentError, "It does not make sense to use both :validator and :enum. Please use either :validator or :enum") + end end describe "#usage" do diff --git a/spec/parser/option_spec.rb b/spec/parser/option_spec.rb index ff81a767a..6152d3b0f 100644 --- a/spec/parser/option_spec.rb +++ b/spec/parser/option_spec.rb @@ -141,6 +141,30 @@ def option(name, options = {}) end.to raise_error(ArgumentError, "An option's default must match its type.") end + it "raises an error if validator is used with boolean option" do + expect do + option = option("foo", :type => :boolean, :validator => proc {}, :validator_desc => 'Validator Description') + end.to raise_error(ArgumentError, "A validator is only supported for type = :string, :numeric, :array or :hash") + end + + it "raises an error if validator does not have call-method" do + expect do + option = option("foo", :type => :string, :validator => Class.new.new, :validator_desc => 'Validator Description') + end.to raise_error(ArgumentError, "A validator needs to respond to #call") + end + + it "raises an error if validator does not have a description" do + expect do + option = option("foo", :type => :string, :validator => proc {}) + end.to raise_error(ArgumentError, "A validator needs a description. Please define :validator_desc") + end + + it "raises an error if validator and enum-option are used together" do + expect do + option = option(:foo, :type => :string, :validator => proc {}, :validator_desc => 'A validator description', :enum => ['a', 'b']) + end.to raise_error(ArgumentError, "It does not make sense to use both :validator and :enum. Please use either :validator or :enum") + end + it "boolean options cannot be required" do expect do option("foo", :required => true, :type => :boolean) diff --git a/spec/parser/options_spec.rb b/spec/parser/options_spec.rb index 9a6a21d24..4ace11893 100644 --- a/spec/parser/options_spec.rb +++ b/spec/parser/options_spec.rb @@ -295,6 +295,18 @@ def remaining expect { parse("--fruit", "orange") }.to raise_error(Thor::MalformattedArgumentError, "Expected '--fruit' to be one of #{enum.join(', ')}; got orange") end + + it "accepts value when value 't match validator" do + create :fruit => Thor::Option.new("fruit", :type => :string, :validator => proc { |v| /orange/ === v }, :validator_desc => 'description') + expect { parse("--fruit", "orange") }.not_to raise_error + end + + it "raises error when value doesn't match validator" do + create :fruit => Thor::Option.new("fruit", :type => :string, :validator => proc { |v| /banana/ === v }, :validator_desc => 'description') + expect { parse("--fruit", "orange") }.to raise_error(Thor::MalformattedArgumentError, + "Expected '--fruit=orange' to return true for `:validator`, but it returns false" + ) + end end describe "with :boolean type" do @@ -368,6 +380,18 @@ def remaining it "must not allow the same hash key to be specified multiple times" do expect {parse("--attributes", "name:string", "name:integer")}.to raise_error(Thor::MalformattedArgumentError, "You can't specify 'name' more than once in option '--attributes'; got name:string and name:integer") end + + it "accepts value when value 't match validator" do + create :attributes => Thor::Option.new("attributes", :type => :hash, :validator => proc { |k,v| /name/ === k && /string/ === v}, :validator_desc => 'description') + expect { parse("--attributes", "name:string") }.not_to raise_error + end + + it "raises error when value doesn't match validator" do + create :attributes => Thor::Option.new("attributes", :type => :hash, :validator => proc { |k,v| /name/ === k && /longstring/ === v}, :validator_desc => 'description') + expect { parse("--attributes", "name:string") }.to raise_error(Thor::MalformattedArgumentError, + "Expected '--attributes=name:string' to return true for `:validator`, but it returns false" + ) + end end describe "with :array type" do @@ -386,6 +410,18 @@ def remaining it "must not mix values with other switches" do expect(parse("--attributes", "a", "b", "c", "--baz", "cool")["attributes"]).to eq(%w[a b c]) end + + it "accepts value when value 't match validator" do + create :attributes => Thor::Option.new("attributes", :type => :array, :validator => proc { |v| v.to_i < 4 }, :validator_desc => 'description') + expect { parse("--attributes", "1", "2", "3") }.not_to raise_error + end + + it "raises error when value doesn't match validator" do + create :attributes => Thor::Option.new("attributes", :type => :array, :validator => proc { |v| v.to_i > 3 }, :validator_desc => 'description') + expect { parse("--attributes", "1", "4") }.to raise_error(Thor::MalformattedArgumentError, + "Expected '--attributes=[1, ...]' to return true for `:validator`, but it returns false" + ) + end end describe "with :numeric type" do @@ -412,7 +448,18 @@ def remaining expect { parse("--limit", "3") }.to raise_error(Thor::MalformattedArgumentError, "Expected '--limit' to be one of #{enum.join(', ')}; got 3") end - end + it "accepts value when value 't match validator" do + create :limit => Thor::Option.new("limit", :type => :numeric, :validator => proc { |v| v == 3 }, :validator_desc => 'description') + expect { parse("--limit", "3") }.not_to raise_error + end + + it "raises error when value doesn't match validator" do + create :limit => Thor::Option.new("limit", :type => :numeric, :validator => proc { |v| v < 3 }, :validator_desc => 'description') + expect { parse("--limit", "3") }.to raise_error(Thor::MalformattedArgumentError, + "Expected '--limit=3' to return true for `:validator`, but it returns false" + ) + end + end end end From 270f9bf3c80a7319e3ca24c942cd3c2220864e1b Mon Sep 17 00:00:00 2001 From: Max Meyer Date: Tue, 12 May 2015 19:29:24 +0200 Subject: [PATCH 2/4] Move code to find a switch to single method --- lib/thor/parser/arguments.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/thor/parser/arguments.rb b/lib/thor/parser/arguments.rb index 4a351c77e..8cab9b791 100644 --- a/lib/thor/parser/arguments.rb +++ b/lib/thor/parser/arguments.rb @@ -84,6 +84,12 @@ def current_is_value? peek && peek.to_s !~ /^-/ end + def for_switch(name) + if @switches.is_a?(Hash) && switch = @switches[name] + yield(switch) + end + end + # Runs through the argument array getting strings that contains ":" and # mark it as a hash: # @@ -100,7 +106,7 @@ def parse_hash(name) while current_is_value? && peek.include?(":") key, value = shift.split(":", 2) - if @switches.is_a?(Hash) && switch = @switches[name] + for_switch name do |switch| if switch.validator && !switch.validator.call(key, value) fail MalformattedArgumentError, "Expected '#{name}=#{key}:#{value}' to return true for `:validator`, but it returns false" end @@ -127,7 +133,7 @@ def parse_array(name) while current_is_value? value = shift - if @switches.is_a?(Hash) && switch = @switches[name] + for_switch name do |switch| if switch.validator && !switch.validator.call(value) fail MalformattedArgumentError, "Expected '#{name}=[#{value}, ...]' to return true for `:validator`, but it returns false" end @@ -151,7 +157,7 @@ def parse_numeric(name) value = $&.index(".") ? shift.to_f : shift.to_i - if @switches.is_a?(Hash) && switch = @switches[name] + for_switch name do |switch| if switch.validator && !switch.validator.call(value) fail MalformattedArgumentError, "Expected '#{name}=#{value}' to return true for `:validator`, but it returns false" elsif switch.enum && !switch.enum.include?(value) @@ -172,7 +178,7 @@ def parse_string(name) nil else value = shift - if @switches.is_a?(Hash) && switch = @switches[name] # rubocop:disable AssignmentInCondition + for_switch name do |switch| if switch.validator && !switch.validator.call(value) fail MalformattedArgumentError, "Expected '#{name}=#{value}' to return true for `:validator`, but it returns false" elsif switch.enum && !switch.enum.include?(value) From 7d8d2163587df0155c0ded4f0a5d8e942886e9c8 Mon Sep 17 00:00:00 2001 From: Max Meyer Date: Tue, 12 May 2015 19:49:01 +0200 Subject: [PATCH 3/4] Added documentation for validator and validator_desc --- lib/thor.rb | 17 +++++++++-------- lib/thor/base.rb | 32 ++++++++++++++++++-------------- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/lib/thor.rb b/lib/thor.rb index d687d3bb6..a72046912 100644 --- a/lib/thor.rb +++ b/lib/thor.rb @@ -137,14 +137,15 @@ def method_options(options = nil) # options:: Described below. # # ==== Options - # :desc - Description for the argument. - # :required - If the argument is required or not. - # :default - Default value for this argument. It cannot be required and have default values. - # :aliases - Aliases for this option. - # :type - The type of the argument, can be :string, :hash, :array, :numeric or :boolean. - # :banner - String to show on usage notes. - # :hide - If you want to hide this option from the help. - # + # :desc - Description for the argument. + # :required - If the argument is required or not. + # :default - Default value for this argument. It cannot be required and have default values. + # :aliases - Aliases for this option. + # :type - The type of the argument, can be :string, :hash, :array, :numeric or :boolean. + # :banner - String to show on usage notes. + # :hide - If you want to hide this option from the help. + # :validator - Check value of argument (:string, :array, :numeric), Check key/value of argument (hash). Needs to respond to #call. + # :validator_desc - Document which keys/values the validator accepts def method_option(name, options = {}) scope = if options[:for] find_and_refresh_command(options[:for]).options diff --git a/lib/thor/base.rb b/lib/thor/base.rb index 7ab947818..cf3935822 100644 --- a/lib/thor/base.rb +++ b/lib/thor/base.rb @@ -195,12 +195,14 @@ def strict_args_position?(config) #:nodoc: # options:: Described below. # # ==== Options - # :desc - Description for the argument. - # :required - If the argument is required or not. - # :optional - If the argument is optional or not. - # :type - The type of the argument, can be :string, :hash, :array, :numeric. - # :default - Default value for this argument. It cannot be required and have default values. - # :banner - String to show on usage notes. + # :desc - Description for the argument. + # :required - If the argument is required or not. + # :optional - If the argument is optional or not. + # :type - The type of the argument, can be :string, :hash, :array, :numeric. + # :default - Default value for this argument. It cannot be required and have default values. + # :banner - String to show on usage notes. + # :validator - Check value of argument (:string, :array, :numeric), Check key/value of argument (hash). Needs to respond to #call. + # :validator_desc - Document which keys/values the validator accepts # # ==== Errors # ArgumentError:: Raised if you supply a required argument after a non required one. @@ -261,14 +263,16 @@ def class_options(options = nil) # options:: Described below. # # ==== Options - # :desc:: -- Description for the argument. - # :required:: -- If the argument is required or not. - # :default:: -- Default value for this argument. - # :group:: -- The group for this options. Use by class options to output options in different levels. - # :aliases:: -- Aliases for this option. Note: Thor follows a convention of one-dash-one-letter options. Thus aliases like "-something" wouldn't be parsed; use either "\--something" or "-s" instead. - # :type:: -- The type of the argument, can be :string, :hash, :array, :numeric or :boolean. - # :banner:: -- String to show on usage notes. - # :hide:: -- If you want to hide this option from the help. + # :desc:: -- Description for the argument. + # :required:: -- If the argument is required or not. + # :default:: -- Default value for this argument. + # :group:: -- The group for this options. Use by class options to output options in different levels. + # :aliases:: -- Aliases for this option. Note: Thor follows a convention of one-dash-one-letter options. Thus aliases like "-something" wouldn't be parsed; use either "\--something" or "-s" instead. + # :type:: -- The type of the argument, can be :string, :hash, :array, :numeric or :boolean. + # :banner:: -- String to show on usage notes. + # :hide:: -- If you want to hide this option from the help. + # :validator:: -- Check value of argument (:string, :array, :numeric), Check key/value of argument (hash). Needs to respond to #call. + # :validator_desc:: -- Document which keys/values the validator accepts # def class_option(name, options = {}) build_option(name, options, class_options) From ac76fbe36361e671d593801ec707a35ed14964a7 Mon Sep 17 00:00:00 2001 From: Max Meyer Date: Tue, 12 May 2015 20:21:19 +0200 Subject: [PATCH 4/4] Output validations when help is invoked --- lib/thor/base.rb | 1 + spec/fixtures/script.thor | 6 ++++++ spec/thor_spec.rb | 4 ++++ 3 files changed, 11 insertions(+) diff --git a/lib/thor/base.rb b/lib/thor/base.rb index cf3935822..7b9ecf8d5 100644 --- a/lib/thor/base.rb +++ b/lib/thor/base.rb @@ -524,6 +524,7 @@ def print_options(shell, options, group_name = nil) list << item list << ["", "# Default: #{option.default}"] if option.show_default? list << ["", "# Possible values: #{option.enum.join(', ')}"] if option.enum + list << ["", "# Validation: #{option.validator_description}"] if option.validator_description end end diff --git a/spec/fixtures/script.thor b/spec/fixtures/script.thor index 6ea1c1e71..4b516d551 100644 --- a/spec/fixtures/script.thor +++ b/spec/fixtures/script.thor @@ -91,6 +91,12 @@ END [name, options, args] end + method_option :my_option, :type => :string, :validator => proc { |v| v == 'hello world' }, :validator_desc => 'Needs to be "hello world"' + desc 'with_validator', 'Check option with validator' + def with_validator + [options] + end + class AnotherScript < Thor desc "baz", "do some bazing" def baz diff --git a/spec/thor_spec.rb b/spec/thor_spec.rb index 55450dc25..ecfd2ff50 100644 --- a/spec/thor_spec.rb +++ b/spec/thor_spec.rb @@ -384,6 +384,10 @@ def shell END end + it "outputs information about validations as well" do + expect(capture(:stdout) { MyScript.command_help(shell, "with_validator") }).to include 'Validation' + end + it "raises an error if the command can't be found" do expect do MyScript.command_help(shell, "unknown")