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

Add the option stringify_keys to #as_json #153

Merged
merged 3 commits into from
Jul 15, 2020
Merged

Add the option stringify_keys to #as_json #153

merged 3 commits into from
Jul 15, 2020

Conversation

fabioperrella
Copy link
Contributor

@fabioperrella fabioperrella commented Jul 8, 2020

Hi folks! This is my 1st PR in this project.

If I did something wrong, please tell me!!

Motivation

fix #151

Proposal

  • Add HashUtils.deep_transform_keys copied from the activesupport source code

I chose to not add activesupport as a dependency because it is good to have the least number of dependencies!

  • Use HashUtils.deep_transform_keys to transform all the keys of the hash generated by as_json to a String

I chose to add the option stringify_keys on #as_json for backward compatibility. I also added info about it on README

@martijnvermaat
Copy link

The Travis tests fail because pry-byebug depends on Ruby 2.4. I suggest to separate the pry-byebug and example_status_persistence_file_path changes into separate PRs so they can be discussed and possibly merged independently.

This class is only instance inside mp3_parser to call #to_h
@fabioperrella
Copy link
Contributor Author

The Travis tests fail because pry-byebug depends on Ruby 2.4. I suggest to separate the pry-byebug and example_status_persistence_file_path changes into separate PRs so they can be discussed and possibly merged independently.

@martijnvermaat done! I will create another PR with these commits

Copy link
Contributor

@linkyndy linkyndy left a comment

Choose a reason for hiding this comment

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

Nice effort, Fabio! 🎉

Clean code, nice approach, concise tests. Keep it up!

README.md Outdated Show resolved Hide resolved
lib/hash_utils.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/attributes_json.rb Outdated Show resolved Hide resolved
lib/attributes_json.rb Outdated Show resolved Hide resolved
lib/hash_utils.rb Show resolved Hide resolved

result = FormatParser::ZIPParser.new.call(fi_io).as_json(stringify_keys: true)
expect(
result['entries'].flat_map { |entry| entry.keys.map(&:class) }.uniq
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest making the expectations a bit less "smart". When dealing with tests it is usually a much more pleasant experience to be visually decoding the code under test, and not the test itself. In this instance the amount of functional sugar used occludes the fact that we want to verify that all keys we are scanning are kind_of(String). Can we use a more pedestrian each_pair (or alike) to the same effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I agree the tests should be less "smart"!

Are you suggesting something like this?

result['entries'].each do |entry|
  entry.each do |key, value|
    expect(key).to be_a(String)
  end
end

I couldn't see a simpler way other than that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that's perfect! If you opt for #each you might want to group the key-value using () though 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I've seen, each_pair is an alias to each, isn't it?

you might want to group the key-value using () though

I didn't get it. Could you give me an example pls?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! I think some_hash.each do |(key, value)| is required instead of some_hash.each do |key, value| so that the array argument to the block is expanded into two variables. Or you can use each_pair instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool! I didn't know about this parenthesis rule for block variables!

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 for the long discussion about it, but I found interesting to know how it works and I think it's good to share it with you!

From what I saw, it is not necessary to use the () here. It would be necessary if the hash contained arrays as values and I wanted to access their values like this:

image

In this case, there are no arrays inside the hashes, and I just want to check their keys, so it makes no difference using ():

image

And also makes no difference using each or each_with_pair given they are aliases!

image

spec/hash_utils_spec.rb Show resolved Hide resolved
lib/parsers/mp3_parser.rb Show resolved Hide resolved
lib/hash_utils.rb Show resolved Hide resolved
@fabioperrella fabioperrella force-pushed the fix-151 branch 5 times, most recently from 219fd89 to a000efa Compare July 13, 2020 18:59
@fabioperrella
Copy link
Contributor Author

tks @julik for all the comments!!

@julik
Copy link
Contributor

julik commented Jul 13, 2020

🎩 My pleasure! Will approve once tests are green

@fabioperrella
Copy link
Contributor Author

🎩 My pleasure! Will approve once tests are green

I messed up a test! Now that I fixed it, it will be green! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return a more uniform structure in as_json
6 participants