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

Adding Hash#compact and Hash#compact! methods #13632

Merged
merged 1 commit into from Jan 9, 2014

Conversation

@tinogomes
Copy link
Contributor

tinogomes commented Jan 8, 2014

Example:

hash = { a: true, b: false, c: nil }
hash.compact # => { a: true, b: false }

Benchmark for better solution:

https://gist.github.com/tinogomes/8332883

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 8, 2014

Not sure.

The name seems awkward to me. compact means remove the key-value pair with nil as key, or remove the key-value pair with nil as value?

In my the snippet below is clear about the intention.

my_hash.reject! { |_, v| v.nil? }

Also, I had a hard time thinking in which cases I'd use this code, and I could not see any usage in regular application.

@tinogomes
Copy link
Contributor Author

tinogomes commented Jan 8, 2014

@rafaelfranca I named as compact inspired by the behavior of Array#compact that is to remove the nil values. We can rename? I do not know a better name right now.

For my usage:

    def standard_button(text, title=text, disabled_text=nil, class_style='submitForward')
      submit_options = :title => title, :class => class_style
      submit_options['data-disable-with'] = disabled_text unless disabled_text.nil?

      submit_tag text, submit_options
   end

instead:

    def standard_button(text, title=text, disabled_text=nil, class_style='submitForward')
      submit_tag text, {:title => title, :class => class_style, 'data-disable-with' => disabled_text}.compact
    end

yes, I can use reject {|_,v| v.nil? }, but repeating this expression for all my codebase, I feel something is wrong.

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 8, 2014

Interesting usage. I think we have some code like this even inside Rails.

I'll delegate to @dhh who is better naming things than I 😄

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 8, 2014

As a side note, given your use case, Rails has you covered:

<%= submit_tag 'zomg', :title => 'title', :class => 'class_style', 'data-disable-with' => nil %>

Would output:

<input class="class_style" name="commit" title="title" type="submit" value="zomg" />

Rails already ignores attributes containing nil values, not generating them on the output html.

@tinogomes
Copy link
Contributor Author

tinogomes commented Jan 8, 2014

@carlosantoniodasilva

Nice to know that Rails already ignore nil values for Helpers.

@dhh
Copy link
Member

dhh commented Jan 8, 2014

I'm sympathetic to this idea. I even think the name parity with Array#compact makes sense. I do remember writing similar code in the past. So 👍 on name and method from me.

@rafaelfranca
rafaelfranca reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated
class Hash
# Compact a hash rejecting all nil values
#
# hash = { :a => true, :b => false, :c => nil}

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jan 9, 2014

Member

Use Ruby 1.9 syntax

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@rafaelfranca
rafaelfranca reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated

protected
def reject_nil_values_lambda
lambda {|key, value| value.nil? }

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jan 9, 2014

Member

Space after the {

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@rafaelfranca
rafaelfranca reviewed Jan 9, 2014
View changes
activesupport/test/core_ext/hash_ext_test.rb Outdated
@@ -865,6 +867,32 @@ def test_except_with_mocha_expectation_on_original
original.expects(:delete).never
original.except(:a)
end

def test_compact
hash_contain_nil_value = @symbols.merge({:z => nil})

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jan 9, 2014

Member

We don't need the { and use the 1.9 syntax

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍 I just follow the file pattern.

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 9, 2014

Great! Lets get this in.

@tinogomes we need to upgrade the active_support guides and we need a CHANGELOG entry.

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated
# hash.compact # => { :a => true, :b => false}
# hash # => { :a => true, :b => false, :c => nil}
def compact
self.reject(&reject_nil_values_lambda)

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jan 9, 2014

Member

You should be able to use dup.compact!, right?

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated
@@ -0,0 +1,24 @@
class Hash
# Compact a hash rejecting all nil values

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jan 9, 2014

Member

Compacts a hash rejecting all +nil+ values.

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated
self.reject(&reject_nil_values_lambda)
end

# Replace the hash with only non nil values

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jan 9, 2014

Member

How about: Removes all elements containing +nil+ values from the hash.

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated
# Replace the hash with only non nil values
#
# hash = { :a => true, :b => false, :c => nil}
# hash.compact # => { :a => true, :b => false}

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jan 9, 2014

Member

The example should use compact!

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@tinogomes
Copy link
Contributor Author

tinogomes commented Jan 9, 2014

@carlosantoniodasilva @rafaelfranca active_support guides and CHANGELOG updated and all your suggestions done!

@egilburg
egilburg reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash.rb Outdated
@@ -5,3 +5,4 @@
require 'active_support/core_ext/hash/keys'
require 'active_support/core_ext/hash/reverse_merge'
require 'active_support/core_ext/hash/slice'
require 'active_support/core_ext/hash/compact'

This comment has been minimized.

Copy link
@egilburg

egilburg Jan 9, 2014

Contributor

put this in alphabetical order with other requires

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@egilburg
egilburg reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated
def reject_nil_values_lambda
lambda { |_, value| value.nil? }
end
end

This comment has been minimized.

Copy link
@egilburg

egilburg Jan 9, 2014

Contributor

newline

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@egilburg
egilburg reviewed Jan 9, 2014
View changes
activesupport/lib/active_support/core_ext/hash/compact.rb Outdated
# hash # => { a: true, b: false, c: nil}
# { c: nil }.compact # => {}
def compact
self.reject(&reject_nil_values_lambda)

This comment has been minimized.

Copy link
@egilburg

egilburg Jan 9, 2014

Contributor

imo implementation is so simple that it doesn't warrant extra method which body is pretty much as complex as its name. plus the slight performance penalty of a lambda allocation and extra method call

also, don't need self. here, it's implied.

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@tinogomes
Copy link
Contributor Author

tinogomes commented Jan 9, 2014

@egilburg all done!

@vipulnsward
vipulnsward reviewed Jan 9, 2014
View changes
guides/source/active_support_core_extensions.md Outdated
@@ -2907,6 +2907,16 @@ The method `with_indifferent_access` returns an `ActiveSupport::HashWithIndiffer

NOTE: Defined in `active_support/core_ext/hash/indifferent_access.rb`.

### Compacting

The methods `compact` and `compact!` return an Hash without items with `nil` value.

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward Jan 9, 2014

Member

return a Hash

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

👍

@vipulnsward
Copy link
Member

vipulnsward commented Jan 9, 2014

@tinogomes you'll need to squash your commits.

# hash # => { a: true, b: false, c: nil}
# { c: nil }.compact # => {}
def compact
self.select { |_, value| !value.nil? }

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward Jan 9, 2014

Member

why select here and reject below? We can have the same implementation at both the places?

This comment has been minimized.

Copy link
@tinogomes

tinogomes Jan 9, 2014

Author Contributor

Benchmark for better solution:

https://gist.github.com/tinogomes/8332883

@@ -1,3 +1,7 @@
* Added `Hash#compact` and `Hash#compact!` for removing items with nil value from hash.

This comment has been minimized.

Copy link
@robin850

robin850 Jan 9, 2014

Member

It's just a tiny proposal here but I would rather say:

* Bring Array's `#compact` and `#compact!` methods to `Hash` to remove nil values

What do you think ?

Also, could you squash your commits like @vipulnsward requested ? You can run git rebase -i HEAD~8 and replace all picks with squash apart for the first line.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jan 9, 2014

Member

The implementation is not the same so I'd use Added as it is now.

  * Adding Hash#compact and Hash#compact! methods
  * Using Ruby 1.9 syntax on documentation
  * Updating guides for `Hash#compact` and `Hash#compact!` methods
  * Updating CHANGELOG for ActiveSupport
  * Removing unecessary protected method and lambda for `Hash#compact` implementations
  * Performing `Hash#compact` implementation - https://gist.github.com/tinogomes/8332883
  * fixing order position
  * Fixing typo
@tinogomes
Copy link
Contributor Author

tinogomes commented Jan 9, 2014

squashed!

rafaelfranca added a commit that referenced this pull request Jan 9, 2014
Adding Hash#compact and Hash#compact! methods
@rafaelfranca rafaelfranca merged commit a67f25d into rails:master Jan 9, 2014
@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 9, 2014

❤️ 💚 💙 💛 💜

@tinogomes
Copy link
Contributor Author

tinogomes commented Jan 9, 2014

👍

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.