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

Improve where chaining docs [ci-skip] #45352

Merged
merged 1 commit into from Jun 14, 2022

Conversation

ghiculescu
Copy link
Member

https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-where only discusses where.not in the "no argument" section. A WhereChain accepts not, missing, or associated, but you have to click through to https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods/WhereChain.html to learn that.

This PR just updates the docs in https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-where to discuss all the chaining options.

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.

Nice improvement! 👍

@@ -722,12 +722,28 @@ def left_outer_joins!(*args) # :nodoc:
# === no argument
#
# If no argument is passed, #where returns a new instance of WhereChain, that
# can be chained with #not to return a new relation that negates the where clause.
# can be chained with #not, #missing, or #associated.
Copy link
Member

Choose a reason for hiding this comment

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

Since these methods are defined on WhereChain, they will actually be rendered as unformatted text, rather than links or inline code. (The current documentation is similarly broken: https://api.rubyonrails.org/v7.0.3/classes/ActiveRecord/QueryMethods.html#method-i-where.)

We can prefix them with WhereChain:

Suggested change
# can be chained with #not, #missing, or #associated.
# can be chained with WhereChain#not, WhereChain#missing, or WhereChain#associated.

Or we can use rdoc-ref links:

Suggested change
# can be chained with #not, #missing, or #associated.
# can be chained with {not}[rdoc-ref:WhereChain#not],
# {missing}[rdoc-ref:WhereChain#missing], or
# {associated}[rdoc-ref:WhereChain#associated].

The former are more verbose when rendered, but the latter are not rendered as inline code (i.e. no highlight, no monospace). I think I lean towards the former.

Copy link
Member

Choose a reason for hiding this comment

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

Drive-by thought inspired by the above comment, without having actually reviewed the PR: "chained with #not" -> "chained as {where.not}[rdoc-ref..]"? (I don't recall, but assume from context, that we can't get both monospace and a custom link in rdoc?)

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall, but assume from context, that we can't get both monospace and a custom link in rdoc?

At least, I have not figured out how to do so. From memory, the following didn't work:

  • {<tt>method_name</tt>}[rdoc-ref:...]
  • {+method_name+}[rdoc-ref:...]
  • <tt>{method_name}[rdoc-ref:...]</tt> (=> rendered literally as code)
  • +{method_name}[rdoc-ref:...]+ (=> exceeds the limited support for quoting with +)

Copy link
Member

Choose a reason for hiding this comment

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

Should we open an issue on rdoc for this? It seems like this would be pretty useful

Copy link
Member

Choose a reason for hiding this comment

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

@skipkayhil Go for it! 🚀 I had added an item on my TODO list to investigate and potentially submit a PR, but I don't know when or if I would get to it.

activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
@ghiculescu
Copy link
Member Author

Thanks @jonathanhefner - I updated accordingly. Good call.

Is there an easy way to test what the rdoc will come out looking like?

@simi
Copy link
Contributor

simi commented Jun 14, 2022

Is there an easy way to test what the rdoc will come out looking like?

Locally? You can run bundle exec rake rdoc and open doc/rdoc/index.html.

@jonathanhefner jonathanhefner changed the title Improve where chaining docs Improve where chaining docs [ci-skip] Jun 14, 2022
https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-where only discusses `where.not` in the "no argument" section. A `WhereChain` accepts `not`, `missing`, or `associated`, but you have to click through to https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods/WhereChain.html to learn that. So this PR just updates the docs in https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-where to discuss all the chaining options.

Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
@jonathanhefner
Copy link
Member

jonathanhefner commented Jun 14, 2022

Thank you, @ghiculescu! 🙌

(Backported to 7-0-stable.)

@ghiculescu ghiculescu deleted the wherechain-docs branch June 15, 2022 01:47
jonathanhefner added a commit that referenced this pull request Jun 25, 2022
Improve `where` chaining docs [ci-skip]

(cherry picked from commit 962a1dd)
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

5 participants