Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validator to method_options with type :string, :numeric, :array and :hash #485

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions lib/thor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,15 @@ def method_options(options = nil)
# options<Hash>:: 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
Expand Down
33 changes: 19 additions & 14 deletions lib/thor/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,14 @@ def strict_args_position?(config) #:nodoc:
# options<Hash>:: 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.
Expand Down Expand Up @@ -261,14 +263,16 @@ def class_options(options = nil)
# options<Hash>:: 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. <b>Note:</b> 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. <b>Note:</b> 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)
Expand Down Expand Up @@ -520,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

Expand Down
38 changes: 30 additions & 8 deletions lib/thor/parser/argument.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {})
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
39 changes: 34 additions & 5 deletions lib/thor/parser/arguments.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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:
#
Expand All @@ -99,6 +105,13 @@ def parse_hash(name)

while current_is_value? && peek.include?(":")
key, value = shift.split(":", 2)

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
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
Expand All @@ -117,7 +130,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

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
end

array << value
end
array
end

Expand All @@ -133,11 +156,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)

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)
fail MalformattedArgumentError, "Expected '#{name}' to be one of #{switch.enum.join(', ')}; got #{value}"
end
end

value
end

Expand All @@ -151,8 +178,10 @@ def parse_string(name)
nil
else
value = shift
if @switches.is_a?(Hash) && switch = @switches[name] # rubocop:disable AssignmentInCondition
if switch.enum && !switch.enum.include?(value)
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)
fail MalformattedArgumentError, "Expected '#{name}' to be one of #{switch.enum.join(', ')}; got #{value}"
end
end
Expand Down
11 changes: 11 additions & 0 deletions lib/thor/parser/option.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions spec/fixtures/script.thor
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions spec/parser/argument_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 24 additions & 0 deletions spec/parser/option_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
49 changes: 48 additions & 1 deletion spec/parser/options_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is missing tests to arguments_spec like those one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you expect to be tested:

  • Option valditaor also works if arguments are given?
  • Adds validator to arguments as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You changed lib/thor/argument.rb to add validator support already. You add tests on this file to make sure when parsing the options the validators are respected, but there are no tests for when an argument is being parsed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca I'd love to see this code merged, and have some time available to work on it. I'm not understanding what kind of tests you would like to see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for the argument parser that now also accepts validators.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca The argument parser seems contradictory. It accepts an Array of Argument objects, then assigns those to a @switches variable, then later checks if that variable contains a Hash. The iteration within the file also seems very strange, and it's difficult to see how the "enum" argument functionality could be working. The argument code seems to need revision and clarification.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca Is this the test that is missing? Perhaps I have misunderstood: benkoshy@b9a735d

(btw thank you for your open source contributions)

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
Expand All @@ -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
Expand All @@ -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
4 changes: 4 additions & 0 deletions spec/thor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down