Skip to content

[DOCS] Improve Documentation for ActiveRecord's order Method [ci-skip]#43018

Merged
jonathanhefner merged 1 commit into
rails:mainfrom
ChaelCodes:document-order
Aug 19, 2021
Merged

[DOCS] Improve Documentation for ActiveRecord's order Method [ci-skip]#43018
jonathanhefner merged 1 commit into
rails:mainfrom
ChaelCodes:document-order

Conversation

@ChaelCodes

@ChaelCodes ChaelCodes commented Aug 15, 2021

Copy link
Copy Markdown
Contributor

Summary

Added documentation around the order method and what the acceptable parameters are. Strings have some interesting functionality where they raise an error if anything more complicated than a column name or function wrapping a column are passed. This wasn't documented, so let's fix that!

Other Information

Old New
Old documentation New Documentation

@ChaelCodes ChaelCodes changed the title Improve Documentation for ActiveRecord's order Method [DOCS]Improve Documentation for ActiveRecord's order Method Aug 15, 2021
@ChaelCodes ChaelCodes changed the title [DOCS]Improve Documentation for ActiveRecord's order Method [DOCS] Improve Documentation for ActiveRecord's order Method Aug 15, 2021
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated

@jonathanhefner jonathanhefner left a comment

Copy link
Copy Markdown
Member

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! 😃

Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment on lines 393 to 394

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# column names and simple function(column_name) expressions with optional
# ASC/DESC modifiers are allowed.
# column names and simple <code>function(column_name)</code> expressions
# with optional +ASC+/+DESC+ modifiers are allowed.

Also, what do you think about moving this paragraph up above the code examples?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought this location transitioned nicely to Arel, and meant they had an understanding of what string values passed to order looked like, before adding the complication of when to use Arel, but I'm fine with moving it up to the first description.

Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
@ChaelCodes

Copy link
Copy Markdown
Contributor Author

Updated to reflect all code review comments, and rebased to drop the commit "fix spelling of management in the changelog", because main is now fixed.

@jonathanhefner jonathanhefner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more thing. Then, when you are happy with the changes, would you mind squashing the commits?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, this is my fault -- this needs to be

Suggested change
# Applies an +ORDER BY+ clause to a query.
# Applies an <code>ORDER BY</code> clause to a query.

Because of the space:

> require "rdoc"
> RDoc::Markup::ToHtml.new(RDoc::Options.new).convert("+ORDER BY+ vs +ORDERBY+")
=> "\n<p>+ORDER BY+ vs <code>ORDERBY</code></p>\n"

@p8 p8 Aug 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at some similar methods in this file, I'm not sure we should use ORDER BY here.
None of the other examples are using SQL in the description.

It also seems we use mostly relation instead of query.
This makes sense I guess, because a relation is returned.

reverse_order
https://github.com/rails/rails/blob/8807b93841ddd1a7fa2b2f44f02e1f690f3d9cc9/activerecord/lib/active_record/relation/query_methods.rb#L1151
reorder
https://github.com/rails/rails/blob/8807b93841ddd1a7fa2b2f44f02e1f690f3d9cc9/activerecord/lib/active_record/relation/query_methods.rb#L440
limit
https://github.com/rails/rails/blob/8807b93841ddd1a7fa2b2f44f02e1f690f3d9cc9/activerecord/lib/active_record/relation/query_methods.rb#L876
Where
https://github.com/rails/rails/blob/8807b93841ddd1a7fa2b2f44f02e1f690f3d9cc9/activerecord/lib/active_record/relation/query_methods.rb#L590-L591

Any objections if we change it to:

Suggested change
# Applies an +ORDER BY+ clause to a query.
# Applies an order clause to the relation.

Or maybe, since multiple orders can be added:

Suggested change
# Applies an +ORDER BY+ clause to a query.
# Adds an order clause to the relation.

Sorry for the bikeshed 🙇

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at some similar methods in this file, I'm not sure we should use ORDER BY here.
None of the other examples are using SQL in the description.

There are a few that do:

# Returns a new relation expressing WHERE + NOT condition according to
# the conditions in the arguments.

# Forces eager loading by performing a LEFT OUTER JOIN on +args+:

# Second: Modifies the SELECT statement for the query so that only certain
# fields are retrieved:

# Allows to specify an order by a specific set of values. Depending on your
# adapter this will either use a CASE statement or a built-in function.

# Allows to specify a HAVING clause. Note that you can't use HAVING
# without also specifying a GROUP clause.

That said, I don't have a strong preference. 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed 👍. I’ll leave it up to @ChaelCodes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think including the ORDER BY provides more information than just saying order but I'm 50/50 on query vs relation, so I'll replace it with relation next time I'm at my computer.

@p8 p8 Aug 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we are sticking with the ORDER BY I guess it makes sense to also stick with query 🙂 .

@ChaelCodes

Copy link
Copy Markdown
Contributor Author

Squashed!

Comment thread activerecord/lib/active_record/relation/query_methods.rb Outdated
@p8

p8 commented Aug 19, 2021

Copy link
Copy Markdown
Member

Thanks @ChaelCodes ! This looks ready to go for me.. 🚢 🙌
Could you squash commits again?
I guess @jonathanhefner already approved?

@ChaelCodes

Copy link
Copy Markdown
Contributor Author

@p8 squashed and building. Sorry for the short message, it's early here.

@p8

p8 commented Aug 19, 2021

Copy link
Copy Markdown
Member

@ChaelCodes no worries 😄 . Thanks for the quick replies!

Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
@jonathanhefner jonathanhefner changed the title [DOCS] Improve Documentation for ActiveRecord's order Method [DOCS] Improve Documentation for ActiveRecord's order Method [ci-skip] Aug 19, 2021
@jonathanhefner jonathanhefner merged commit 074c7f5 into rails:main Aug 19, 2021
@jonathanhefner

Copy link
Copy Markdown
Member

Ack, I noticed +function(column_name)+ didn't get changed to <code>function(column_name)</code> (per #43018 (comment)). Since we've already done multiple iterations, I did a quick force push to fix it. I hope you don't mind, @ChaelCodes. 🙏

Thank you again, @ChaelCodes! And thank you, @p8, for reviewing!

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.

4 participants