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

Delegate some additional methods in querying.rb #24315

Merged
merged 1 commit into from
Mar 29, 2016

Conversation

kenta-s
Copy link
Contributor

@kenta-s 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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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

@kenta-s
Copy link
Contributor Author

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 add_empty_to_finder_methods branch from 07807c9 to 04d1caf Compare March 29, 2016 10:50
@kenta-s
Copy link
Contributor Author

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 Add empty? to ActiveRecord::FinderMethods Delegate some additional methods in querying.rb Mar 29, 2016
assert !Topic.one?
Topic.delete_all
assert !Topic.one?
Topic.new.save
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be Topic.create!

@kenta-s kenta-s force-pushed the add_empty_to_finder_methods branch from 04d1caf to 691242d Compare March 29, 2016 12:33
@kenta-s
Copy link
Contributor Author

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 add_empty_to_finder_methods branch from 691242d to 78ea4c5 Compare March 29, 2016 15:01
@kenta-s
Copy link
Contributor Author

kenta-s commented Mar 29, 2016

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

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

@kenta-s thanks for your contribution 👍

@kenta-s kenta-s deleted the add_empty_to_finder_methods branch March 30, 2016 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants