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.

Sign up for GitHub

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

Fix fallback argument for Psych.load #339

Closed
wants to merge 3 commits into from
Closed

Fix fallback argument for Psych.load #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
278e307
Add more test cases for fallback of Psych.load_file
stomar Dec 1, 2017
07bdd21
Add test cases for fallback argument of Psych.load
stomar Dec 1, 2017
b1ac5a4
Fix fallback argument for Psych.load
stomar Dec 1, 2017
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
6 changes: 3 additions & 3 deletions lib/psych.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ module Psych
# Psych.load("---\n foo: bar", symbolize_names: true) # => {:foo=>"bar"}
#
def self.load yaml, filename = nil, fallback = false, symbolize_names: false
Copy link
Member

Choose a reason for hiding this comment

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

How about changing "fallback = false" to "fallback: false"? With that change, users can easily notice fallback's behavior is fixed/changed.

Copy link
Member

@k0kubun k0kubun Dec 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Or another idea is having "legacy_fallback = false, fallback: false" and keeping legacy_fallback's behavior for migration, deprecating legacy_fallback usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fallback: false sounds good to me, if the compatibility break is acceptable and if this doesn't cause problems in any of the other methods that possibly should be considered, too (safe_load, load_stream, and so on).

legacy_fallback doesn't convince me, I would guess it has the exact same problem with hash arguments; also I think fallback is not widely used currently.

Copy link
Member

Choose a reason for hiding this comment

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

fallback: false sounds good to me, if the compatibility break is acceptable and if this doesn't cause problems in any of the other methods that possibly should be considered, too (safe_load, load_stream, and so on).

I agree with it. That should depend on maintainer's decision anyway.

result = parse(yaml, filename, fallback)
result = parse(yaml, filename, FALLBACK.new(fallback))
result = result.to_ruby if result
symbolize_names!(result) if symbolize_names
result
Expand Down Expand Up @@ -482,10 +482,10 @@ def self.load_stream yaml, filename = nil
###
# Load the document contained in +filename+. Returns the yaml contained in
# +filename+ as a Ruby object, or if the file is empty, it returns
# the specified default return value, which defaults to an empty Hash
# the specified +fallback+ return value, which defaults to +false+.
def self.load_file filename, fallback = false
File.open(filename, 'r:bom|utf-8') { |f|
self.load f, filename, FALLBACK.new(fallback)
self.load f, filename, fallback
}
end

Expand Down
24 changes: 24 additions & 0 deletions test/psych/test_psych.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,18 @@ def test_domain_types
assert_equal({ 'hello' => 'world' }, got)
end

def test_load_default_return_value
assert_equal false, Psych.load("")
end

def test_load_with_fallback
assert_equal 42, Psych.load("", "file", 42)
end

def test_load_with_fallback_hash
assert_equal Hash.new, Psych.load("", "file", Hash.new)
end

def test_load_file
Tempfile.create(['yikes', 'yml']) {|t|
t.binmode
Expand All @@ -144,7 +156,19 @@ def test_load_file
}
end

def test_load_file_default_return_value
Tempfile.create(['empty', 'yml']) {|t|
assert_equal false, Psych.load_file(t.path)
}
end

def test_load_file_with_fallback
Tempfile.create(['empty', 'yml']) {|t|
assert_equal 42, Psych.load_file(t.path, 42)
}
end

def test_load_file_with_fallback_hash
Tempfile.create(['empty', 'yml']) {|t|
assert_equal Hash.new, Psych.load_file(t.path, Hash.new)
}
Expand Down