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

Fix JSON.dump overload combination #558

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Conversation

tompng
Copy link
Member

@tompng tompng commented Dec 4, 2023

Fixed this overload

# wrong argument type Hash (expected Integer)
dump({}, io, strict: true)
dump({}, limit, strict: true)

And fixed this NoMethodError

# Works and tested in test code
dump({}, strict: true) # => "{}"
# NoMethodError. was not tested in test code
JSON.dump({}, strict: true) # => undefined method `merge_dump_options' for JSON:Module

@@ -642,12 +639,14 @@ def self.iconv(to, from, string)
string.encode(to, from)
end

private
Copy link
Member Author

Choose a reason for hiding this comment

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

merge_dump_options should be private instance method and also private module function

module_function

def a_private_instance_method_and_public_module_function
end

private # module_function rule is cleared

def a_private_instance_method # not a module function
end

Comment on lines +618 to +621
if anIO.respond_to?(:to_io)
anIO = anIO.to_io
elsif limit.nil? && !anIO.respond_to?(:write)
anIO, limit = nil, anIO
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the inconsistency

dump(obj, io, nil) #=> io.to_io was called
dump(obj, io, 10) #=> io.to_io was not called

And if to_io is called, the result of to_io should be an IO, no need to check for respond_to?(:write)

@hsbt hsbt merged commit f11c570 into ruby:master Dec 5, 2023
60 checks passed
@tompng tompng deleted the json_dump_args branch December 5, 2023 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants