make Hash#extract! more symmetric with Hash#slice #7007

Merged
merged 2 commits into from Oct 12, 2012

Conversation

Projects
None yet
7 participants
@Mik-die
Contributor

Mik-die commented Jul 8, 2012

This version of extract! doesn't return pairs with nil values, if key is absent in receiver.

Also, it returns extracted hash of the same type as reciever (for example, on hash with indifferent access it returns hash with indifferent access)

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 8, 2012

Owner

I agree that using self.class.new is a good idea, to return a hash of the same type. But the other change bugs me a little bit, do you have any particular reasoning why doing it? Do you think it's wrong, can it lead someone to do something wrong, or some other idea? Otherwise I'd argue to not change it. Thanks!

I agree that using self.class.new is a good idea, to return a hash of the same type. But the other change bugs me a little bit, do you have any particular reasoning why doing it? Do you think it's wrong, can it lead someone to do something wrong, or some other idea? Otherwise I'd argue to not change it. Thanks!

@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Jul 9, 2012

Contributor

I can give you two example of irrational behavior of today extract!

params = {:a => 1, :b => 2, :c => 3, :d => 4}
x = params.extract!(:x)
x.empty? ? 'OK' : x
#  => {:x=>nil} 

And, if you think too, that above output is irrational, here is another example

some_AR_model.name # 'James'
params = {:a => 1, :b => 2, :c => 3, :d => 4}
some_AR_model.update_attributes(params.extract!(:name))
some_AR_model.name # nil

I think in the second example more rational is to not touch name attribute, because it's absent in original params hash

Contributor

Mik-die commented Jul 9, 2012

I can give you two example of irrational behavior of today extract!

params = {:a => 1, :b => 2, :c => 3, :d => 4}
x = params.extract!(:x)
x.empty? ? 'OK' : x
#  => {:x=>nil} 

And, if you think too, that above output is irrational, here is another example

some_AR_model.name # 'James'
params = {:a => 1, :b => 2, :c => 3, :d => 4}
some_AR_model.update_attributes(params.extract!(:name))
some_AR_model.name # nil

I think in the second example more rational is to not touch name attribute, because it's absent in original params hash

@drogus

This comment has been minimized.

Show comment Hide comment
@drogus

drogus Jul 11, 2012

Member

I kind of agree with this patch. On the one hand, if you want to extract :x and there is no such key, you could get nil, but on the other hand - no key does not mean the same as nil key. The problem is, extract! behaves that way for a long time and this change is not quite backwards compatible.

Member

drogus commented Jul 11, 2012

I kind of agree with this patch. On the one hand, if you want to extract :x and there is no such key, you could get nil, but on the other hand - no key does not mean the same as nil key. The problem is, extract! behaves that way for a long time and this change is not quite backwards compatible.

@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Jul 20, 2012

Contributor

I don't understand, which sensible code we can break by this patch..

I can assume some code, that'll be broken after patch:

x = params.extract!(:x)
do_something if x.present? 
do_something2 if x.has_key?(:x) 

In this code both methods do_something and do_something2 run without patch, and don't run with patch. But this is not sensible code, because now x will always be present and always had key :x, therefore both conditions are redundant.

On the other hand, if we try to access key :x, in both cases we get nil value

Contributor

Mik-die commented Jul 20, 2012

I don't understand, which sensible code we can break by this patch..

I can assume some code, that'll be broken after patch:

x = params.extract!(:x)
do_something if x.present? 
do_something2 if x.has_key?(:x) 

In this code both methods do_something and do_something2 run without patch, and don't run with patch. But this is not sensible code, because now x will always be present and always had key :x, therefore both conditions are redundant.

On the other hand, if we try to access key :x, in both cases we get nil value

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 21, 2012

Owner

@Mik-die hey, lets get this merged? Can you please add two changelog entries, one for each change? It'd be good to ensure it works with nil values as well, so adding more assertions for that would be nice. Just ping me afterwards and I'll merge, thanks!

@Mik-die hey, lets get this merged? Can you please add two changelog entries, one for each change? It'd be good to ensure it works with nil values as well, so adding more assertions for that would be nice. Just ping me afterwards and I'll merge, thanks!

@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Sep 21, 2012

Contributor

Hi @carlosantoniodasilva ! I made changelog entries, added 1 test and rebased relatively current master.

Contributor

Mik-die commented Sep 21, 2012

Hi @carlosantoniodasilva ! I made changelog entries, added 1 test and rebased relatively current master.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Sep 21, 2012

Owner
@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Sep 21, 2012

Contributor

@fxn sure, I'll add commit in docrails after this PR will be merged

Contributor

Mik-die commented Sep 21, 2012

@fxn sure, I'll add commit in docrails after this PR will be merged

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Sep 21, 2012

Owner

If you don't mind, I'd prefer the edit in this pull request. Otherwise the patch is incomplete.

docrails is only for quick doc fixes, not for later documentation.

Owner

fxn commented Sep 21, 2012

If you don't mind, I'd prefer the edit in this pull request. Otherwise the patch is incomplete.

docrails is only for quick doc fixes, not for later documentation.

@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Sep 21, 2012

Contributor

@fxn ok.

And also I note, that +slice+ now converts keys for hashes with indifferent access, so I'll add this feature too (+ test for it)

Contributor

Mik-die commented Sep 21, 2012

@fxn ok.

And also I note, that +slice+ now converts keys for hashes with indifferent access, so I'll add this feature too (+ test for it)

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Sep 21, 2012

Owner

Awesome, thanks :).

Owner

fxn commented Sep 21, 2012

Awesome, thanks :).

@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Sep 21, 2012

Contributor

@carlosantoniodasilva @fxn I added more tests and docs

Contributor

Mik-die commented Sep 21, 2012

@carlosantoniodasilva @fxn I added more tests and docs

@bradleypriest

View changes

activesupport/CHANGELOG.md
@@ -1,5 +1,17 @@
## Rails 4.0.0 (unreleased) ##
+* Hash#extract! returns only those keys that present in the reciever.
+

This comment has been minimized.

Show comment Hide comment
@bradleypriest

bradleypriest Sep 21, 2012

Contributor

A small typo here /recieve/receive/g

@bradleypriest

bradleypriest Sep 21, 2012

Contributor

A small typo here /recieve/receive/g

@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Sep 21, 2012

Contributor

@bradleypriest fixed this, thanks!

Contributor

Mik-die commented Sep 21, 2012

@bradleypriest fixed this, thanks!

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 7, 2012

Owner

This pull request needs a rebase

Owner

rafaelfranca commented Oct 7, 2012

This pull request needs a rebase

@Mik-die

This comment has been minimized.

Show comment Hide comment
@Mik-die

Mik-die Oct 7, 2012

Contributor
Contributor

Mik-die commented Oct 7, 2012

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Oct 12, 2012

Member

Can we merge this?

Member

schneems commented Oct 12, 2012

Can we merge this?

rafaelfranca added a commit that referenced this pull request Oct 12, 2012

Merge pull request #7007 from Mik-die/hash_extract
make Hash#extract! more symmetric with Hash#slice

@rafaelfranca rafaelfranca merged commit e68b97a into rails:master Oct 12, 2012

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 12, 2012

Owner

Thanks

Owner

rafaelfranca commented Oct 12, 2012

Thanks

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Oct 12, 2012

Owner

Awesome, thanks guys!

Awesome, thanks guys!

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