Skip to content

Conversation

martinjaimem
Copy link
Contributor

Summary

Update the documentation of the guides to reflect that #any? and #many? use limit.

@rails-bot rails-bot bot added the docs label Nov 29, 2021
@p8 p8 added the ready PRs ready to merge label Nov 29, 2021
Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. When you are happy with the changes, would you mind squashing the commits?

Comment on lines -2185 to -2186
Order.shipped.any? # => SELECT 1 AS one FROM orders WHERE orders.status = 0
Order.shipped.many? # => SELECT COUNT(*) FROM orders WHERE orders.status = 0
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for removing these? Would adding LIMIT clauses to these be more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed them because I thought it would be cleaner, though I wasn't sure tbh 👍

I re-added them and squashed commits 👍

Copy link
Member

Choose a reason for hiding this comment

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

I removed them because I thought it would be cleaner, though I wasn't sure tbh

I see what you mean. The rendered output is getting a little crowded. What if we omit the SQL aliases, and put the SQL statements on separate lines? (I've added suggestions to illustrate.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it 🏄 .

You are right, it looks cleaner now without the aliases.

I guess this final version is better in terms of readability, we are on the other hand saying that:

  • it is not necessary for the reader to know the exact query executed from the guides (as we are omitting the aliases)
  • we are saying in a way that it is more useful that the reader knows there is a limit in the query than knowing there are aliases in the real query

Copy link
Member

Choose a reason for hiding this comment

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

  • it is not necessary for the reader to know the exact query executed from the guides (as we are omitting the aliases)
  • we are saying in a way that it is more useful that the reader knows there is a limit in the query than knowing there are aliases in the real query

I think that is fine. 👍 Those aliases don't really provide much information. But it can be useful to know that Rails is smart enough to include a LIMIT, so that we don't have to worry about adding it ourselves.

@jonathanhefner jonathanhefner changed the title Update documentation #any? #many? Update documentation #any? #many? [ci-skip] Nov 29, 2021
@jonathanhefner jonathanhefner removed the ready PRs ready to merge label Nov 29, 2021
@martinjaimem martinjaimem force-pushed the update-documentation-many-any branch from afb97f4 to e909767 Compare November 29, 2021 17:16
Comment on lines -2185 to -2186
Order.shipped.any? # => SELECT 1 AS one FROM orders WHERE orders.status = 0
Order.shipped.many? # => SELECT COUNT(*) FROM orders WHERE orders.status = 0
Copy link
Member

Choose a reason for hiding this comment

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

I removed them because I thought it would be cleaner, though I wasn't sure tbh

I see what you mean. The rendered output is getting a little crowded. What if we omit the SQL aliases, and put the SQL statements on separate lines? (I've added suggestions to illustrate.)

@martinjaimem martinjaimem force-pushed the update-documentation-many-any branch from e909767 to 5d985f2 Compare November 29, 2021 18:30
@jonathanhefner jonathanhefner merged commit 8a419e8 into rails:main Nov 29, 2021
@jonathanhefner
Copy link
Member

Thank you, @martinjaimem! 🎉

@martinjaimem martinjaimem deleted the update-documentation-many-any branch November 29, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants