Skip to content

Commit

Permalink
Output warning instead of raising an exepection on default type mismatch
Browse files Browse the repository at this point in the history 
See #435 (comment)

Closes #533.
  • Loading branch information
@sferik
sferik committed Nov 27, 2016
1 parent a074db0 commit 605897b
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 4 deletions.
3 changes: 2 additions & 1 deletion lib/thor/parser/option.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ def validate_default_type!
@default.class.name.downcase.to_sym
end

raise ArgumentError, "An option's default must match its type." unless default_type == @type
# TODO: This should raise an ArgumentError in a future version of Thor
warn "Expected #{@type} default value for '#{switch_name}'; got #{@default.inspect} (#{default_type})" unless default_type == @type
end

def dasherized?
Expand Down
6 changes: 3 additions & 3 deletions spec/parser/option_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,9 @@ def option(name, options = {})
end

it "raises an error if default is inconsistent with type" do
expect do
option("foo", :type => :numeric, :default => "bar")
end.to raise_error(ArgumentError, "An option's default must match its type.")
expect(capture(:stderr) do
option("foo_bar", :type => :numeric, :default => "baz")
end.chomp).to eq('Expected numeric default value for \'--foo-bar\'; got "baz" (string)')
end

it "does not raises an error if default is an symbol and type string" do
Expand Down

1 comment on commit 605897b

@matthewd
Copy link
Member

Choose a reason for hiding this comment

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

Even a warning seems quite disruptive here 😕

The problem, IMO, is that the warning appears directly inside the user-facing UI, because of what this library is used for:

% rails generate scaffold Product title:string description:text image_url:string price:decimal
Expected string default value for '--jbuilder'; got true (boolean)
      invoke  active_record
      create    db/migrate/20161127114807_create_products.rb
      create    app/models/product.rb
[..]

It's complaining to someone who can't fix it, and likely doesn't even know what it's talking about.

Perhaps it could go back to raising, but only when a (default off, for compatibility) config setting is enabled?

That way developers can enable it, gain the safety, and fix their code, without immediately disrupting the workflow of their downstream users.

Please sign in to comment.