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

Calls readers only when a key exists for Mash#update #465

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

laertispappas
Copy link

@laertispappas laertispappas commented Sep 30, 2018

Addresses: #464

Checks if key is present in deep_update before calling any reader method. Calling a reader method will execute the default block and we now prevent it.

@laertispappas laertispappas force-pushed the merge_default_blk branch 6 times, most recently from c5c87b4 to 381343d Compare September 30, 2018 17:39
@@ -1,5 +1,6 @@
require 'spec_helper'

# rubocop: disable Metrics/BlockLength
Copy link
Member

Choose a reason for hiding this comment

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

Run rubocop -a ; rubocop --auto-gen-config instead.

Copy link
Author

Choose a reason for hiding this comment

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

@dblock I have problems making this to work. The above changes some files but does not make the build to pass (see my last commit.)

@michaelherold
Copy link
Member

It looks like you ran Rubocop with a newer version than the one we lint with, which is why CI is failing. You can use bundle exec in front of the Rubocop commands to lock down to the version that we use.

@@ -209,7 +209,7 @@ def deep_merge(other_hash, &blk)
def deep_update(other_hash, &blk)
other_hash.each_pair do |k, v|
key = convert_key(k)
if regular_reader(key).is_a?(Mash) && v.is_a?(::Hash)
if key?(key) && regular_reader(key).is_a?(Mash) && v.is_a?(::Hash)
Copy link
Member

Choose a reason for hiding this comment

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

Simple fix! Since I'm looking at the line, I feel like we should swap the order of the v.is_a? check and the reader check, but that's not something we need to address in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

+1 @michaelherold

@laertispappas I know this is outside of the scope here, but care to make this change so we don't forget? It's a lot faster to do is_a? than to invoke regular_reader. Thanks.

@laertispappas
Copy link
Author

@michaelherold thanks for the feedback. Running with bundle exec I get diffs on all files (see the last commit). It's getting really frustrating...

@dblock
Copy link
Member

dblock commented Oct 1, 2018

Looks like a previous run added all those frozen literals, so you should undo that.

Check that rubocop -v shows the same version as in the Gemfile, 0.52.1.

Otherwise get a clean branch, make sure to bundle install and bundle exec rubocop -a ; bundle exec rubucop --auto-gen-config, which should make (almost) no changes, then apply your fixes and rerun robocop again. Also rebase against master, since #466 touched upon the rubocop setup.

@dblock
Copy link
Member

dblock commented Oct 1, 2018

If you are still having trouble, I can pick the changes from here. LMK!

@laertispappas
Copy link
Author

@dblock thanks for the feedback. I will retry since #466 is merged. I will let you know. Thanks

@laertispappas laertispappas changed the title Calls readers only when a key exists Calls readers only when a key exists for Mash#update Oct 1, 2018
@laertispappas
Copy link
Author

@dblock okay skipping rubocop line length on specs saved me :)
thanks.

@dblock
Copy link
Member

dblock commented Oct 2, 2018

@laertispappas Try to squash this too. Not required but nice.

@laertispappas
Copy link
Author

@dblock Done. Thanks.

@dblock dblock merged commit d097d02 into hashie:master Oct 2, 2018
@dblock
Copy link
Member

dblock commented Oct 2, 2018

I merged this. Care to take care of https://github.com/intridea/hashie/pull/465/files#r221469352 in a new PR?

@laertispappas
Copy link
Author

laertispappas commented Oct 2, 2018

@dblock Thanks and I don't have a strong argument on this. But why? Personally I kind of have key value pairs in my head (note key first) instead of value key. So what would be the value / difference on the change?

@dblock
Copy link
Member

dblock commented Oct 3, 2018

if key?(key) && regular_reader(key).is_a?(Mash) && v.is_a?(::Hash)

vs.

if v.is_a?(::Hash) && key?(key) && regular_reader(key).is_a?(Mash)

Avoids calling an expensive method regular_reader or a less expensive key? method when v is a ::Hash, so just faster

@laertispappas
Copy link
Author

laertispappas commented Oct 3, 2018

@dblock Ack thanks. Will follow with the reverse order.

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.

3 participants