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

Added documentation for ActiveRecord::Relation#pick [skip ci] #48404

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

soartec-lab
Copy link
Contributor

Motivation / Background

This Pull Request has been created because there is no explanation of ActiveRecord::Relation#pick in the guide.

Detail

This Pull Request changes to add the documentation for ActiveRecord::Relation#pick to the guide.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the docs label Jun 6, 2023

[`pick`][] can be used to query single or multiple columns from the underlying table of a model. It accepts a list of column names as an argument and returns the first one of the specified column values ​​with corresponding data type.

pick is a short-hand for `relation.limit(1).pluck(*column_names).first`, which makes it possible to replace code like:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pick is a short-hand for `relation.limit(1).pluck(*column_names).first`, which makes it possible to replace code like:
`pick` is a short-hand for `relation.limit(1).pluck(*column_names).first`, which makes it possible to replace code like:

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 will handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 also created a PR to change the ActiveRecord::Relation#pluck guide.

#48465

guides/source/active_record_querying.md Show resolved Hide resolved
@soartec-lab soartec-lab force-pushed the task/add-doc-to-activerecord-pick branch from f5c23e7 to 870f7bb Compare June 6, 2023 13:41
@soartec-lab
Copy link
Contributor Author

@p8
Thank you for your review.
I have corrected what you pointed out.
could you to review again?

@soartec-lab soartec-lab requested a review from p8 June 6, 2023 13:45
@@ -2256,6 +2256,24 @@ irb> assoc.unscope(:includes).pluck(:id)

[`pluck`]: https://api.rubyonrails.org/classes/ActiveRecord/Calculations.html#method-i-pluck

### `pick`

[`pick`][] can be used to query single or multiple columns from the underlying table of a model. It accepts a list of column names as an argument and returns the first one of the specified column values ​​with corresponding data type.
Copy link
Member

Choose a reason for hiding this comment

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

I think this was copied and modified from the documentation of pluck.
The explanation in the API docs is a lot clearer to me.
What do you think about using some of the API docs wording?
For example:

Suggested change
[`pick`][] can be used to query single or multiple columns from the underlying table of a model. It accepts a list of column names as an argument and returns the first one of the specified column values ​​with corresponding data type.
[`pick`][] can be used to pick the value(s) from the named column(s) in the current relation. This is primarily useful when you have a relation that’s already narrowed down to a single row. It accepts a list of column names as an argument and returns the first one of the specified column values ​​with corresponding data type.

PS. I'd also accept a seperate PR to reword the first paragraph of pluck to something more similar to the API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good!
Indeed, I would change this as I thought the wording in the API docs was clearer.
And, I will create a separate PR for changes to pluck

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supported by 900bf10

Changed the description to use some of the wording from the API documentation. Also, Added nuances to the main useful cases.

Comment on lines 2265 to 2274
```ruby
Customer.limit(1).pluck(:id).first
```

with:

```ruby
Customer.pick(:id)
```
Copy link
Member

Choose a reason for hiding this comment

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

The example shows how easy it is to shorten the code with pick.
But I think it misses the nuance that pick "is primarily useful when you have a relation that’s already narrowed down to a single row.".

Suggested change
```ruby
Customer.limit(1).pluck(:id).first
```
with:
```ruby
Customer.pick(:id)
```
```ruby
Customer.where(id: 1).pluck(:id).first
```
with:
```ruby
Customer.where(id: 1).pick(:id)
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, an important nuance was missing. I will reflect the suggested source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supported by 9904db0

@soartec-lab
Copy link
Contributor Author

@p8

Thanks for your review.
I have reflected the suggestions you gave me.
In addition, the ActiveRecord::Relation#pluck guide is also addressed in #48465.

Please check it out.

@soartec-lab soartec-lab requested a review from p8 June 14, 2023 02:56
guides/source/active_record_querying.md Outdated Show resolved Hide resolved
- Add nuances of the primarily useful cases to the code examples
- Changed description to use some wording from API docs
@soartec-lab soartec-lab force-pushed the task/add-doc-to-activerecord-pick branch from b717293 to 48a26f5 Compare June 19, 2023 08:42
@soartec-lab soartec-lab requested a review from p8 June 19, 2023 08:42
@soartec-lab
Copy link
Contributor Author

@p8

thank you for making sure.I have modified your last suggestion.
I also squashed all commits.

@p8
Copy link
Member

p8 commented Jun 19, 2023

Thanks @soartec-lab !

@p8 p8 merged commit 676fdb1 into rails:main Jun 19, 2023
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.

None yet

2 participants