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

Replace merge by merge! #1386

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@glaucocustodio
Contributor

glaucocustodio commented May 5, 2016

As described here, merge! is 24x faster than merge.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock May 5, 2016

Member

See build and rubocop errors, there's notably one you can do a lot faster than merge!.

Generally you cannot be modifying the hash being passed in, so not all your changes of merge for merge! are ok. Finally if you're going to be doing merge!, you don't need to assign the result, it's modifying the hash in-place.

Member

dblock commented May 5, 2016

See build and rubocop errors, there's notably one you can do a lot faster than merge!.

Generally you cannot be modifying the hash being passed in, so not all your changes of merge for merge! are ok. Finally if you're going to be doing merge!, you don't need to assign the result, it's modifying the hash in-place.

@glaucocustodio

This comment has been minimized.

Show comment
Hide comment
@glaucocustodio

glaucocustodio May 6, 2016

Contributor

Take a look now please @dblock.

Contributor

glaucocustodio commented May 6, 2016

Take a look now please @dblock.

@@ -32,7 +32,7 @@ def each(&block)
end
def delete(name, opts = {})
options = opts.merge(value: 'deleted', expires: Time.at(0))
options = opts.merge!(value: 'deleted', expires: Time.at(0))

This comment has been minimized.

@dblock

dblock May 6, 2016

Member

This modifies opts being passed in, you cannot do that.

@dblock

dblock May 6, 2016

Member

This modifies opts being passed in, you cannot do that.

This comment has been minimized.

@glaucocustodio

glaucocustodio May 6, 2016

Contributor

Why? Both options and opts are local variables, they aren't used somewhere else except there.

@glaucocustodio

glaucocustodio May 6, 2016

Contributor

Why? Both options and opts are local variables, they aren't used somewhere else except there.

This comment has been minimized.

@dblock

dblock May 6, 2016

Member

When you say local, local to the class? Clearly here opts is not a local variable.

2.2.4 :001 > def doit(opts)
2.2.4 :002?>     opts[:foobar] = 1
2.2.4 :003?>   end
 => :doit 
2.2.4 :004 > 
2.2.4 :005 >   opts = {}
 => {} 
2.2.4 :006 > doit(opts)
 => 1 
2.2.4 :007 > p opts
{:foobar=>1}
 => {:foobar=>1} 

Modifying a hash within the function causes a side effect of modifying the caller's hash. So it's bad practice, finding bugs caused by such side effect is really hard.

@dblock

dblock May 6, 2016

Member

When you say local, local to the class? Clearly here opts is not a local variable.

2.2.4 :001 > def doit(opts)
2.2.4 :002?>     opts[:foobar] = 1
2.2.4 :003?>   end
 => :doit 
2.2.4 :004 > 
2.2.4 :005 >   opts = {}
 => {} 
2.2.4 :006 > doit(opts)
 => 1 
2.2.4 :007 > p opts
{:foobar=>1}
 => {:foobar=>1} 

Modifying a hash within the function causes a side effect of modifying the caller's hash. So it's bad practice, finding bugs caused by such side effect is really hard.

This comment has been minimized.

@glaucocustodio

glaucocustodio May 6, 2016

Contributor

😯 I thought the arguments were copied, but they are the same to those which are passed to the method. Very nice explanation 👏.

@glaucocustodio

glaucocustodio May 6, 2016

Contributor

😯 I thought the arguments were copied, but they are the same to those which are passed to the method. Very nice explanation 👏.

@@ -51,7 +51,7 @@ def desc(description, options = {}, &config_block)
end
options = config_class.settings
else
options = options.merge(description: description)
options[:description] = description

This comment has been minimized.

@dblock

dblock May 6, 2016

Member

This modifies options passed in, you cannot do that.

@dblock

dblock May 6, 2016

Member

This modifies options passed in, you cannot do that.

This comment has been minimized.

@glaucocustodio

glaucocustodio May 6, 2016

Contributor

Same here, no?!

@glaucocustodio

glaucocustodio May 6, 2016

Contributor

Same here, no?!

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock May 6, 2016

Member

Most of these continue to have the same problem: modifying the Hash argument being passed in. This will have unpredictable side effects - It might be faster, but it only works because we get lucky or we have to think hard about where that hash came from.

Member

dblock commented May 6, 2016

Most of these continue to have the same problem: modifying the Hash argument being passed in. This will have unpredictable side effects - It might be faster, but it only works because we get lucky or we have to think hard about where that hash came from.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock May 6, 2016

Member

Lets close this, thanks for hanging in here @glaucocustodio.

Member

dblock commented May 6, 2016

Lets close this, thanks for hanging in here @glaucocustodio.

@dblock dblock closed this May 6, 2016

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