Skip to content
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
1 change: 1 addition & 0 deletions bundler/lib/bundler/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class Settings
bin
cache_path
console
default_cli_command
gem.ci
gem.github_username
gem.linter
Expand Down
7 changes: 7 additions & 0 deletions bundler/lib/bundler/settings/validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,13 @@ def self.validate!(key, value, settings)
fail!(key, value, "`#{other_key}` is current set to #{other_setting.inspect}", "the `#{conflicting.join("`, `")}` groups conflict")
end
end

rule %w[default_cli_command], "default_cli_command must be either 'install' or 'cli_help'" do |key, value, settings|
valid_values = %w[install cli_help]
if !value.nil? && !valid_values.include?(value.to_s)
fail!(key, value, "must be one of: #{valid_values.join(", ")}")
end
end
end
end
end
24 changes: 24 additions & 0 deletions bundler/spec/bundler/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -351,4 +351,28 @@
expect(settings["mirror.https://rubygems.org/"]).to eq("http://example-mirror.rubygems.org")
end
end

describe "default_cli_command validation" do
it "accepts 'install' as a valid value" do
expect { settings.set_local("default_cli_command", "install") }.not_to raise_error
end

it "accepts 'cli_help' as a valid value" do
expect { settings.set_local("default_cli_command", "cli_help") }.not_to raise_error
end

it "rejects invalid values" do
expect { settings.set_local("default_cli_command", "invalid") }.to raise_error(
Bundler::InvalidOption,
/Setting `default_cli_command` to "invalid" failed:\n - default_cli_command must be either 'install' or 'cli_help'\n - must be one of: install, cli_help/
)
end

it "rejects nil values" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this test to match the implementation and name it it "accepts nil values".

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the code in the rule checking for !value.nil? in order to not raise an error it's not even triggered by this test, because we Settings#set_local returns early if the previous value of the setting (nil in this case), matches the value passed to #set_local (nil, too).

So I'd add an extra test it "accepts nil values for unsetting an existing value of the setting" that first sets settings.set_local("default_cli_command", "install") and then checks that settings.set_local("default_cli_command", nil) does not raise an error.

expect { settings.set_local("default_cli_command", nil) }.to raise_error(
Bundler::InvalidOption,
/Setting `default_cli_command` to nil failed:\n - default_cli_command must be either 'install' or 'cli_help'\n - must be one of: install, cli_help/
)
end
end
end
Loading