Merge pull request #36412 from robotdana/compact_blank

Add compact_blank shortcut for reject(&:blank?)

11 files changed, 118 insertions, 13 deletions

diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb
index 6fbd52dd51..920ae52f2b 100644
--- a/actionpack/lib/action_controller/metal/strong_parameters.rb
+++ b/actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -749,6 +749,18 @@ module ActionController
     end
     alias_method :delete_if, :reject!
 
+    # Returns a new instance of <tt>ActionController::Parameters</tt> without the blank values.
+    # Uses Object#blank? for determining if a value is blank.
+    def compact_blank
+      reject { |_k, v| v.blank? }
+    end
+
+    # Removes all blank values in place and returns self.
+    # Uses Object#blank? for determining if a value is blank.
+    def compact_blank!
+      reject! { |_k, v| v.blank? }
+    end
+
     # Returns values that were assigned to the given +keys+. Note that all the
     # +Hash+ objects will be converted to <tt>ActionController::Parameters</tt>.
     def values_at(*keys)
diff --git a/actionpack/test/controller/parameters/mutators_test.rb b/actionpack/test/controller/parameters/mutators_test.rb
index 31ee7964f5..ffffd2996f 100644
--- a/actionpack/test/controller/parameters/mutators_test.rb
+++ b/actionpack/test/controller/parameters/mutators_test.rb
@@ -127,4 +127,22 @@ class ParametersMutatorsTest < ActiveSupport::TestCase
   test "deep_transform_keys! retains unpermitted status" do
     assert_not_predicate @params.deep_transform_keys! { |k| k }, :permitted?
   end
+
+  test "compact_blank retains permitted status" do
+    @params.permit!
+    assert_predicate @params.compact_blank, :permitted?
+  end
+
+  test "compact_blank retains unpermitted status" do
+    assert_not_predicate @params.compact_blank, :permitted?
+  end
+
+  test "compact_blank! retains permitted status" do
+    @params.permit!
+    assert_predicate @params.compact_blank!, :permitted?
+  end
+
+  test "compact_blank! retains unpermitted status" do
+    assert_not_predicate @params.compact_blank!, :permitted?
+  end
 end
diff --git a/actionview/lib/action_view/testing/resolvers.rb b/actionview/lib/action_view/testing/resolvers.rb
index 539bedcdf0..99443663dd 100644
--- a/actionview/lib/action_view/testing/resolvers.rb
+++ b/actionview/lib/action_view/testing/resolvers.rb
@@ -41,7 +41,7 @@ module ActionView #:nodoc:
           )
         end
 
-        templates.sort_by { |t| -t.identifier.match(/^#{query}$/).captures.reject(&:blank?).size }
+        templates.sort_by { |t| -t.identifier.match(/^#{query}$/).captures.compact_blank.size }
       end
   end
 
diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index c3d4eab562..891b50d160 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -56,7 +56,7 @@ module ActiveRecord
       def ids_writer(ids)
         primary_key = reflection.association_primary_key
         pk_type = klass.type_for_attribute(primary_key)
-        ids = Array(ids).reject(&:blank?)
+        ids = Array(ids).compact_blank
         ids.map! { |i| pk_type.cast(i) }
 
         records = klass.where(primary_key => ids).index_by do |r|
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index 405fecb603..ef1eef6b69 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -476,12 +476,12 @@ module ActiveRecord
       # distinct queries, and requires that the ORDER BY include the distinct column.
       # See https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html
       def columns_for_distinct(columns, orders) # :nodoc:
-        order_columns = orders.reject(&:blank?).map { |s|
+        order_columns = orders.compact_blank.map { |s|
           # Convert Arel node to string
           s = s.to_sql unless s.is_a?(String)
           # Remove any ASC/DESC modifiers
           s.gsub(/\s+(?:ASC|DESC)\b/i, "")
-        }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }
+        }.compact_blank.map.with_index { |column, i| "#{column} AS alias_#{i}" }
 
         (order_columns << super).join(", ")
       end
diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb
index df26f67c6e..0732b69f81 100644
--- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb
+++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb
@@ -50,7 +50,7 @@ module ActiveRecord
 
         # Converts the given URL to a full connection hash.
         def to_hash
-          config = raw_config.reject { |_, value| value.blank? }
+          config = raw_config.compact_blank
           config.map { |key, value| config[key] = uri_parser.unescape(value) if value.is_a? String }
           config
         end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
index 0062952667..628a609521 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -555,13 +555,13 @@ module ActiveRecord
         # PostgreSQL requires the ORDER BY columns in the select list for distinct queries, and
         # requires that the ORDER BY include the distinct column.
         def columns_for_distinct(columns, orders) #:nodoc:
-          order_columns = orders.reject(&:blank?).map { |s|
+          order_columns = orders.compact_blank.map { |s|
               # Convert Arel node to string
               s = s.to_sql unless s.is_a?(String)
               # Remove any ASC/DESC modifiers
               s.gsub(/\s+(?:ASC|DESC)\b/i, "")
                .gsub(/\s+NULLS\s+(?:FIRST|LAST)\b/i, "")
-            }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }
+            }.compact_blank.map.with_index { |column, i| "#{column} AS alias_#{i}" }
 
           (order_columns << super).join(", ")
         end
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 6a181882ae..6957ba052b 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -138,7 +138,7 @@ module ActiveRecord
     end
 
     def includes!(*args) # :nodoc:
-      args.reject!(&:blank?)
+      args.compact_blank!
       args.flatten!
 
       self.includes_values |= args
@@ -265,7 +265,7 @@ module ActiveRecord
     end
 
     def _select!(*fields) # :nodoc:
-      fields.reject!(&:blank?)
+      fields.compact_blank!
       fields.flatten!
       self.select_values += fields
       self
@@ -965,7 +965,7 @@ module ActiveRecord
 
     def reverse_order! # :nodoc:
       orders = order_values.uniq
-      orders.reject!(&:blank?)
+      orders.compact_blank!
       self.order_values = reverse_sql_order(orders)
       self
     end
@@ -1053,7 +1053,7 @@ module ActiveRecord
           )
           arel.skip(Arel::Nodes::BindParam.new(offset_attribute))
         end
-        arel.group(*arel_columns(group_values.uniq.reject(&:blank?))) unless group_values.empty?
+        arel.group(*arel_columns(group_values.uniq.compact_blank)) unless group_values.empty?
 
         build_order(arel)
 
@@ -1129,7 +1129,7 @@ module ActiveRecord
         association_joins = buckets[:association_join]
         stashed_joins     = buckets[:stashed_join]
         join_nodes        = buckets[:join_node].tap(&:uniq!)
-        string_joins      = buckets[:string_join].delete_if(&:blank?).map!(&:strip).tap(&:uniq!)
+        string_joins      = buckets[:string_join].compact_blank!.map!(&:strip).tap(&:uniq!)
 
         string_joins.map! { |join| table.create_string_join(Arel.sql(join)) }
 
@@ -1227,7 +1227,7 @@ module ActiveRecord
 
       def build_order(arel)
         orders = order_values.uniq
-        orders.reject!(&:blank?)
+        orders.compact_blank!
 
         arel.order(*orders) unless orders.empty?
       end
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 5d6bb7a0b3..f20c7c92e6 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Add `compact_blank` for those times when you want to remove #blank? values from
+    an Enumerable (also `compact_blank!` on Hash, Array, ActionController::Parameters)
+
+    *Dana Sherson*
+
 *   Make ActiveSupport::Logger Fiber-safe. Fixes #36752.
 
     Use `Fiber.current.__id__` in `ActiveSupport::Logger#local_level=` in order
diff --git a/activesupport/lib/active_support/core_ext/enumerable.rb b/activesupport/lib/active_support/core_ext/enumerable.rb
index 4675c41936..728d332306 100644
--- a/activesupport/lib/active_support/core_ext/enumerable.rb
+++ b/activesupport/lib/active_support/core_ext/enumerable.rb
@@ -148,6 +148,41 @@ module Enumerable
       map { |element| element[keys.first] }
     end
   end
+
+  # Returns a new +Array+ without the blank items.
+  # Uses Object#blank? for determining if an item is blank.
+  #
+  #    [1, "", nil, 2, " ", [], {}, false, true].compact_blank
+  #    # =>  [1, 2, true]
+  #
+  #    Set.new([nil, "", 1, 2])
+  #    # => [2, 1] (or [1, 2])
+  #
+  # When called on a +Hash+, returns a new +Hash+ without the blank values.
+  #
+  #    { a: "", b: 1, c: nil, d: [], e: false, f: true }.compact_blank
+  #    #=> { b: 1, f: true }
+  def compact_blank
+    reject(&:blank?)
+  end
+end
+
+class Hash
+  # Hash#reject has its own definition, so this needs one too.
+  def compact_blank #:nodoc:
+    reject { |_k, v| v.blank? }
+  end
+
+  # Removes all blank values from the +Hash+ in place and returns self.
+  # Uses Object#blank? for determining if a value is blank.
+  #
+  #    h = { a: "", b: 1, c: nil, d: [], e: false, f: true }
+  #    h.compact_blank!
+  #    # => { b: 1, f: true }
+  def compact_blank!
+    # use delete_if rather than reject! because it always returns self even if nothing changed
+    delete_if { |_k, v| v.blank? }
+  end
 end
 
 class Range #:nodoc:
@@ -185,4 +220,15 @@ class Array #:nodoc:
       super
     end
   end
+
+  # Removes all blank elements from the +Array+ in place and returns self.
+  # Uses Object#blank? for determining if an item is blank.
+  #
+  #    a = [1, "", nil, 2, " ", [], {}, false, true]
+  #    a.compact_blank!
+  #    # =>  [1, 2, true]
+  def compact_blank!
+    # use delete_if rather than reject! because it always returns self even if nothing changed
+    delete_if(&:blank?)
+  end
 end
diff --git a/activesupport/test/core_ext/enumerable_test.rb b/activesupport/test/core_ext/enumerable_test.rb
index 381b5a1f32..a9bf4b82f4 100644
--- a/activesupport/test/core_ext/enumerable_test.rb
+++ b/activesupport/test/core_ext/enumerable_test.rb
@@ -242,4 +242,28 @@ class EnumerableTests < ActiveSupport::TestCase
     ])
     assert_equal [[5, 99], [15, 0], [10, 50]], payments.pluck(:dollars, :cents)
   end
+
+  def test_compact_blank
+    values = GenericEnumerable.new([1, "", nil, 2, " ", [], {}, false, true])
+
+    assert_equal [1, 2, true], values.compact_blank
+  end
+
+  def test_array_compact_blank!
+    values = [1, "", nil, 2, " ", [], {}, false, true]
+    values.compact_blank!
+
+    assert_equal [1, 2, true], values
+  end
+
+  def test_hash_compact_blank
+    values = { a: "", b: 1, c: nil, d: [], e: false, f: true }
+    assert_equal({ b: 1, f: true }, values.compact_blank)
+  end
+
+  def test_hash_compact_blank!
+    values = { a: "", b: 1, c: nil, d: [], e: false, f: true }
+    values.compact_blank!
+    assert_equal({ b: 1, f: true }, values)
+  end
 end