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

Use Sequel's any_not_empty extension #2856

Merged

Conversation

philippthun
Copy link
Member

@philippthun philippthun commented Jul 6, 2022

There are already several PRs (see below [1]) that change dataset.any? to !dataset.empty? or otherwise mention the difference between these two statements. Whereas the first one fetches all data from the database to then check if the resulting array contains at least one element, the second statement does a select 1 as one from table limit 1 query that has a far better performance.

Fortunately there is a Sequel extension (see [2]) that 'fixes' this misleading behavior; so instead of searching for more places where we could change the implementation, using dataset.any? should now be equivalent to !dataset.empty? and everyone can choose freely which one to use.

[1]
#2117
#2533
#2803
#2855

[2] https://sequel.jeremyevans.net/rdoc-plugins/files/lib/sequel/extensions/any_not_empty_rb.html

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

There are already several PRs (see below [1]) that change dataset.any?
to !dataset.empty? or otherwise mention the difference between these two
statements. Whereas the first one fetches all data from the database to
then check if the resulting array contains at least one element, the
second statement does a 'select 1 as one from table limit 1' query that
has a far better performance.

Fortunately there is a Sequel extension (see [2]) that 'fixes' this
misleading behavior; so instead of searching for more places where we
could change the implementation, using dataset.any? should now be
equivalent to !dataset.empty? and everyone can choose freely which one
to use.

[1]
cloudfoundry#2117
cloudfoundry#2533
cloudfoundry#2803
cloudfoundry#2855

[2] https://sequel.jeremyevans.net/rdoc-plugins/files/lib/sequel/extensions/any_not_empty_rb.html
@philippthun philippthun force-pushed the override-dataset-any branch from 4d80b6e to e075d01 Compare July 11, 2022 13:32
@philippthun philippthun changed the title Override Sequel::Dataset.any? Use Sequel's any_not_empty extension Jul 11, 2022
@philippthun philippthun marked this pull request as ready for review July 11, 2022 14:03
Copy link
Member

@moleske moleske left a comment

Choose a reason for hiding this comment

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

This seems alright to me. Also seems better than trying to write some sort of rubocop type rule that says "are you sure you want to use dataset.any? and not dataset.empty?"

@philippthun philippthun merged commit 5b358a0 into cloudfoundry:main Sep 12, 2022
@philippthun philippthun mentioned this pull request Nov 8, 2022
5 tasks
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.

3 participants