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

Delegate some additional methods in querying.rb #24315

Merged
merged 1 commit into from Mar 29, 2016

Conversation

Projects
None yet
7 participants
@kenta-s
Contributor

kenta-s commented Mar 25, 2016

I have seen many people write code like below so far.

if Person.count.zero?
  # do something
end

if they can code like this,

if Person.empty?
  # do something
end

the code will look more like RoR.
They can use exists? method of course. But I think there should be empty? method as well.

@rails-bot

This comment has been minimized.

rails-bot commented Mar 25, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 28, 2016

@kenta-s I agree that we should delegate empty? to all the same way as any? and many?, but there's no need to add an implementation since there's already one:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation.rb#L279-L288

Can you modify the PR to just have the delegation, the tests and can you also add a CHANGELOG entry - thanks!

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 28, 2016

Also the test case should live in named_scoping_test.rb since that's where the ones for any? and many? live. Try to follow the existing patterns, so your test could be as simple as:

  def test_model_class_should_respond_to_empty
    assert !Topic.empty?
    Topic.delete_all
    assert Topic.empty?
  end

Thanks!

@egilburg

This comment has been minimized.

Contributor

egilburg commented Mar 28, 2016

In this case none? should also be delegated (and perhaps for completeness' sake, one?) as well.

@kenta-s

This comment has been minimized.

Contributor

kenta-s commented Mar 28, 2016

Thank you for your feedback! I will modify this PR to just have the delegation soon. and I will also check none? and one? as well :)

@kenta-s kenta-s force-pushed the kenta-s:add_empty_to_finder_methods branch Mar 29, 2016

@kenta-s

This comment has been minimized.

Contributor

kenta-s commented Mar 29, 2016

@pixeltrix Done. And I also added none? and one? to the delegation as @egilburg pointed out.
I would appreciate any further feedback. Thank you :)

@kenta-s kenta-s changed the title from Add empty? to ActiveRecord::FinderMethods to Delegate some additional methods in querying.rb Mar 29, 2016

@pixeltrix

View changes

activerecord/test/cases/scoping/named_scoping_test.rb Outdated
assert !Topic.one?
Topic.delete_all
assert !Topic.one?
Topic.new.save

This comment has been minimized.

@pixeltrix

pixeltrix Mar 29, 2016

Member

This could be Topic.create!

@kenta-s kenta-s force-pushed the kenta-s:add_empty_to_finder_methods branch Mar 29, 2016

@kenta-s

This comment has been minimized.

Contributor

kenta-s commented Mar 29, 2016

@pixeltrix Done!

@kenta-s kenta-s closed this Mar 29, 2016

@kenta-s kenta-s reopened this Mar 29, 2016

@@ -1,3 +1,7 @@
* Delegate `empty?`, `none?` and `one?`. Now they can be invoked as model class methods.

This comment has been minimized.

@vipulnsward

vipulnsward Mar 29, 2016

Member

An example for each case would be good here.

Topic.empty? # => false
Topic.one? # => true
Topic.none? # => false

@kenta-s kenta-s force-pushed the kenta-s:add_empty_to_finder_methods branch to 78ea4c5 Mar 29, 2016

@kenta-s

This comment has been minimized.

Contributor

kenta-s commented Mar 29, 2016

Updated. @vipulnsward Thank you for your feedback! :)

@pixeltrix pixeltrix merged commit cd736db into rails:master Mar 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 29, 2016

@kenta-s thanks for your contribution 👍

@kenta-s kenta-s deleted the kenta-s:add_empty_to_finder_methods branch Mar 30, 2016

@januszm

This comment has been minimized.

januszm commented on activerecord/lib/active_record/querying.rb in 78ea4c5 Apr 30, 2016

This might have quite big impact on 3rd party libs, empty? is used by blank? and the latter is used in validates_presence_of (validate_each to be precise)

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