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

Use Hash#compact and Hash#compact! from Ruby 2.4 #26868

Merged
merged 1 commit into from Oct 25, 2016

Conversation

Projects
None yet
6 participants
@prathamesh-sonpatki
Member

prathamesh-sonpatki commented Oct 23, 2016

Summary

r? @rafaelfranca

Show outdated Hide outdated activesupport/lib/active_support/core_ext/hash/compact.rb
# { c: true }.compact # => { c: true }
def compact
select { |_, value| !value.nil? }
unless self.instance_methods(false).include?(:compact)

This comment has been minimized.

@vipulnsward

vipulnsward Oct 23, 2016

Member

This should be an explicit check on Hash.instance_methods(false), not sure how the behaviour would be with Hash inherited classes.

@vipulnsward

vipulnsward Oct 23, 2016

Member

This should be an explicit check on Hash.instance_methods(false), not sure how the behaviour would be with Hash inherited classes.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Oct 23, 2016

Member

What is the difference between this and explicit check?

@prathamesh-sonpatki

prathamesh-sonpatki Oct 23, 2016

Member

What is the difference between this and explicit check?

This comment has been minimized.

@vipulnsward

vipulnsward Oct 23, 2016

Member
class Hash
  def puts_self
    puts "Self#{self.class}"
  end
end


class HashImp < Hash

end


HashImp.new.puts_self

=> SelfHashImp

The self picks up the current class. Again, not sure about the implications, its better to be explicit.

@vipulnsward

vipulnsward Oct 23, 2016

Member
class Hash
  def puts_self
    puts "Self#{self.class}"
  end
end


class HashImp < Hash

end


HashImp.new.puts_self

=> SelfHashImp

The self picks up the current class. Again, not sure about the implications, its better to be explicit.

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Oct 23, 2016

Member

Needs CHANGELOG like vipulnsward@7ad4690
to indicate the change it ruby 2.4+

Member

vipulnsward commented Oct 23, 2016

Needs CHANGELOG like vipulnsward@7ad4690
to indicate the change it ruby 2.4+

@prathamesh-sonpatki

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Oct 23, 2016

Member

I did not add changelog because this change is compatible with AS. So no change in public consumption of these methods unlike Enumerable#sum.

Member

prathamesh-sonpatki commented Oct 23, 2016

I did not add changelog because this change is compatible with AS. So no change in public consumption of these methods unlike Enumerable#sum.

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Oct 23, 2016

Member

I did not add changelog because this change is compatible with AS. So no change in public
consumption of these methods unlike Enumerable#sum.

Ideally it would good that they are same, but if there are any differences/bugs that we find in future, it is good to mention what has changed.

Member

vipulnsward commented Oct 23, 2016

I did not add changelog because this change is compatible with AS. So no change in public
consumption of these methods unlike Enumerable#sum.

Ideally it would good that they are same, but if there are any differences/bugs that we find in future, it is good to mention what has changed.

Use Hash#compact and Hash#compact! from Ruby 2.4
- Ruby 2.4 has added Hash#compact and Hash#compact! so we can use it
  now.
- Reference: https://bugs.ruby-lang.org/issues/11818 and https://bugs.ruby-lang.org/issues/12863.
@seuros

This comment has been minimized.

Show comment
Hide comment
@seuros

seuros Oct 24, 2016

Member

@prathamesh-sonpatki I think we can test just once , because the language either implement both or none.

  unless Hash.instance_methods(false).include?(:compact)
   class Hash
      def compact
      # code here
      def compact!
      #code here....
Member

seuros commented Oct 24, 2016

@prathamesh-sonpatki I think we can test just once , because the language either implement both or none.

  unless Hash.instance_methods(false).include?(:compact)
   class Hash
      def compact
      # code here
      def compact!
      #code here....

@rafaelfranca rafaelfranca merged commit 3cc30db into rails:master Oct 25, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:use-hash-compact-from-ruby-24 branch Oct 25, 2016

@vtrkanna

This comment has been minimized.

Show comment
Hide comment
@vtrkanna

vtrkanna Oct 25, 2016

Is this part of ruby 2.4 or rails 5 ?

vtrkanna commented Oct 25, 2016

Is this part of ruby 2.4 or rails 5 ?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented Oct 25, 2016

both

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Oct 25, 2016

Member

Active Support has shipped with Hash#compact since 4.2, this just uses Ruby's built in methods when they come out in 2.4 (so Ruby upgrading users will get the faster version).

Member

kaspth commented Oct 25, 2016

Active Support has shipped with Hash#compact since 4.2, this just uses Ruby's built in methods when they come out in 2.4 (so Ruby upgrading users will get the faster version).

@prathamesh-sonpatki

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki
Member

prathamesh-sonpatki commented Oct 25, 2016

Ruby 2.4

@kaspth

This comment has been minimized.

Show comment
Hide comment
Member

kaspth commented Oct 25, 2016

Jinx! @prathamesh-sonpatki @rafaelfranca 😁

y-yagi added a commit to y-yagi/rails that referenced this pull request Dec 17, 2016

ensure `#compact` of HWIDA to return HWIDA
`Hash#compact` of Ruby native returns new hash.
Therefore, in order to return HWIDA as in the past version, need to
define own `#compact` to HWIDA.

Related: #26868

y-yagi added a commit to y-yagi/rails that referenced this pull request Dec 17, 2016

ensure `#compact` of HWIDA to return HWIDA
`Hash#compact` of Ruby native returns new hash.
Therefore, in order to return HWIDA as in the past version, need to
define own `#compact` to HWIDA.

Related: #26868

matthewd added a commit to matthewd/rails that referenced this pull request Dec 26, 2016

Merge pull request #26868 from prathamesh-sonpatki/use-hash-compact-f…
…rom-ruby-24

Use Hash#compact and Hash#compact! from Ruby 2.4

matthewd added a commit to matthewd/rails that referenced this pull request Dec 27, 2016

Merge pull request #26868 from prathamesh-sonpatki/use-hash-compact-f…
…rom-ruby-24

Use Hash#compact and Hash#compact! from Ruby 2.4

matthewd added a commit to matthewd/rails that referenced this pull request Dec 27, 2016

Merge pull request #26868 from prathamesh-sonpatki/use-hash-compact-f…
…rom-ruby-24

Use Hash#compact and Hash#compact! from Ruby 2.4

y-yagi added a commit to y-yagi/rails that referenced this pull request Jan 6, 2017

ensure `#compact` of HWIDA to return HWIDA
`Hash#compact` of Ruby native returns new hash.
Therefore, in order to return HWIDA as in the past version, need to
define own `#compact` to HWIDA.

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