-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Document where
support for AR normalizes
#49105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Earlopain Thanks for updating this 👍 . This LGTM after squashing your commits.
Also some other small changelog tweak: * Add normalize_value_for to the activerecord changelog * Fix a wrong method reference in the 7.1 release notes
Done! |
In #49105, `where` examples were added to the `normalizes` documentation to demonstrate that normalizations are also applied for `where`. However, as with the `exists?` examples, we should also demonstrate that normalizations are only applied to keyword arguments, not positional arguments. We can also address the original source of the confusion by changing the wording of "finder methods" to "query methods". This commit also removes the tests added in #49105. `normalizes` works at the level of attribute types, so there is no need to test every query method. Testing `find_by` is sufficient. (And, in point of fact, `find_by` is implemented in terms of `where`.)
@@ -856,6 +856,7 @@ | |||
```ruby | |||
class User < ActiveRecord::Base | |||
normalizes :email, with: -> email { email.strip.downcase } | |||
normalizes :phone, with: -> phone { phone.delete("^0-9").delete_prefix("1") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Earlopain This example is a little confusing, how about just phone.strip
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, sorry I was mixing up the previous example because you added the where
below 🤦 sorry!
Motivation / Background
Reading through the changelog for
normalized
made me wonder if it also works withwhere
. It says it works with finder methods, but I don't thinkwhere
is one of those. It works regardless, so I added it to the changelog and made some tests for it.I also noticed some inconsistencies in the changelog, fixed those up too. I hope that's ok to put in one PR.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]