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

Jump to bottom

Add load_file and load_file! methods, with tests. Fixes issue #386. #387

Merged
merged 1 commit into from
Jun 30, 2020
Merged

Add load_file and load_file! methods, with tests. Fixes issue #386. #387

merged 1 commit into from
Jun 30, 2020

Conversation

keithrbennett
Copy link
Contributor

Adds load_file methods analogous to YAML's.

rafaltrojanowski
rafaltrojanowski reviewed Oct 24, 2019
View reviewed changes
lib/json/common.rb Outdated
# Parses the content of a file
# See parse method documentation for more information.
def load_file(filespec, opts = {})
parse(File.read(filespec))
Copy link

Choose a reason for hiding this comment

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

@keithrbennett I'm just wondering, shouldn't we pass options for 'File.read'? It might be useful for supporting different encodings, etc. It's not related but I have a case where I struggle with BOM character in the beginning of the CSV file and by passing encoding I can simply remove those additional bytes. Maybe that would work similarly in JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First I want to thank you for pointing out to me that I was failing to pass the passed opts along to the called methods. I've fixed that and pushed the fix.

Regarding your point about File.read options, do I understand you correctly that you would want to specify options to File.read, not JSON.parse? If so, I'm not sure how we could do that, since we are already using the Ruby hash last parameter feature for the JSON.parse options.

Any strategy to handle options to File.read would need to be balanced with the cost of the complexity added to support it. Can you suggest a way to accommodate this need in the least complex way?

Copy link

Choose a reason for hiding this comment

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

Yes, but I'm happy with current solution. I don't have idea how to solve it in a elegant way. Maybe maintainer can help. And sorry for the oftop, anyway it's better than before :)

Copy link
Member

Choose a reason for hiding this comment

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

Best not mix options. If a user need options, they can do the File.read. FWIW, YAML.load_file has no file option either.

@rafaltrojanowski
Copy link

Thanks @keithrbennett!

marcandre
marcandre reviewed Jun 25, 2020
View reviewed changes
tests/json_common_interface_test.rb
@@ -123,4 +123,43 @@ def test_JSON
assert_equal @json, JSON(@hash)
assert_equal @hash, JSON(@json)
end


def test_load_file
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test with options...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I've added this (and an equivalent function for load_file!), would it suffice?:

  def test_load_file_with_option
    temp_file_containing(@json) do |filespec|
      key_classes = load_file(filespec, symbolize_names: true).keys.map(&:class)
      assert_true (key_classes.include?(Symbol) && (! key_classes.include?(String)))
    end
  end

Also, in order to get the tests to pass I had to rebase master my feature branch. Is it ok to git push -f this branch?

Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to git push -f this branch?

Yes, that's the best thing to do 😄

@marcandre
Copy link
Member

Very good. Only thing remaining would be to squash your commits (although it can be done by maintainers with 'squash and merge')

marcandre
marcandre reviewed Jun 25, 2020
View reviewed changes
tests/json_common_interface_test.rb Outdated

def test_load_shared(method_name)
temp_file_containing(@json) do |filespec|
assert_equal method(method_name).call(filespec), @hash
Copy link
Member

Choose a reason for hiding this comment

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

method(method_name).call(filespec) => public_send(method_name, filespec). I think it's more idiomatic.
Same for test_load_file_with_option_shared

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this change and squashed the commits. Thanks for your guidance, I agree that these are both improvements.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the impressively quick turnaround and good PR!

marcandre
marcandre approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Good PR 👍
LGTM

@hsbt
Copy link
Member

hsbt commented Jun 30, 2020

@keithrbennett Thanks. Could you rebase from this from master branch?

I will merge this targeted json-2.4.0 after releasing json-2.3.1.

@keithrbennett
Copy link
Contributor Author

keithrbennett commented Jun 30, 2020 via email

@hsbt hsbt merged commit e1451ed into ruby:master Jun 30, 2020
@hsbt
Copy link
Member

hsbt commented Jun 30, 2020

Thanks!

@keithrbennett keithrbennett deleted the add-load-file-methods branch July 3, 2020 18:42
@casperisfine casperisfine mentioned this pull request Sep 10, 2020
Merged
@hsbt hsbt mentioned this pull request May 27, 2021
Closed
@sambostock sambostock mentioned this pull request Apr 13, 2023
Merged
@tk0miya tk0miya mentioned this pull request Sep 29, 2024
Merged
tk0miya added a commit to tk0miya/rbs that referenced this pull request Sep 29, 2024
@tk0miya
stdlib: Add types for JSON.load_file and .load_file!
36f84b7 
refs:

* https://rubydoc.info/gems/json/JSON.load_file
* https://github.com/ruby/ruby/blob/master/ext/json/lib/json/common.rb#L242-L262
* ruby/ruby#3581
* https://github.com/flori/json/blob/v2.7.2/lib/json/common.rb#L242-L262
* ruby/json#387
tk0miya added a commit to tk0miya/doctree that referenced this pull request Sep 29, 2024
@tk0miya
json: Add .load_file and .load_file!
6793fc6 
refs:

* https://rubydoc.info/gems/json/JSON.load_file
* https://github.com/ruby/ruby/blob/master/ext/json/lib/json/common.rb#L242-L262
* ruby/ruby#3581
* https://github.com/flori/json/blob/v2.7.2/lib/json/common.rb#L242-L262
* ruby/json#387
@tk0miya tk0miya mentioned this pull request Sep 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaltrojanowski rafaltrojanowski rafaltrojanowski left review comments

@marcandre marcandre marcandre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@keithrbennett @rafaltrojanowski @marcandre @hsbt