[Fix #51044] Deprecate providing a non-boolean argument to #distinct
#51045
Conversation
|
Looks like there are 2 failures in other test cases that pass a non-boolean to Edit: Updated these to not pass a non-boolean into |
joshuay03
force-pushed
the
raise-on-invalid-distinct-args
branch
6 times, most recently
from
1b924d8
to
487a6ef
Compare
|
This might require a deprecation cycle since it’s a breaking change. Similar to #50931 |
joshuay03
force-pushed
the
raise-on-invalid-distinct-args
branch
from
487a6ef
to
b8e6187
Compare
Addressed in these changes. |
joshuay03
changed the title
ArgumentError when providing non-boolean arguments to #distinct#distinct
| @@ -1312,6 +1312,10 @@ def distinct(value = true) | |||
|
|
|||
| # Like #distinct, but modifies relation in place. | |||
| def distinct!(value = true) # :nodoc: | |||
| unless value == true || value == false | |||
There was a problem hiding this comment.
The deprecation should be in the public method, not the private one. Apps shouldn't call distinct! as it's nodoc'd.
That said, I'm not convinced this is worth changing. Rails and Ruby are designed to be truthy and falsey. Even if we validate this one, you're still going to have to explain to juniors or anyone new to the language what truthy/falsey means. Also in 99% of cases, anyone calling distinct is passing no value at all.
I appreciate you sending a PR, but I'm personally against making this change. Not sure if anyone else on Core feels strongly in favor.
There was a problem hiding this comment.
The problem with distinct is that in SQL, distinct takes an argument which is the column you want to unique on (select distinct name from users). But in Rails it's different: User.select(:name).distinct is fine; User.distinct(:name) is wrong.
It's not surprising that (not just junior) developers forget this difference. If you agree that the Rails API is confusing, then I think it's worth doing what we can to make it less confusing.
Rails and Ruby are designed to be truthy and falsey.
I agree with this in general, but do you think there's valid exceptions (and then do you think this is one of them)?
#45229 is another example where technically any truthy argument works, but in practice there's no sensible use case for passing a value other than true or false.
There was a problem hiding this comment.
The deprecation should be in the public method, not the private one. Apps shouldn't call
distinct!as it'snodoc'd.
I was originally raising an ArgumentError and questioned why both shouldn't be consistent, since the private method is utilised by the public method and is tested (to be specific, it had a test that passes in a non-boolean argument).
I left the check where it was when I updated to warn of a deprecation instead so that it's easier for whomever swaps it for a raise in 7.1.3 to just update the one line, which will raise an error in both methods as I had it before.
I'm happy to move it to just the public method if we also don't want the private method to raise in 7.1.3.
joshuay03
force-pushed
the
raise-on-invalid-distinct-args
branch
from
b8e6187
to
9da87fb
Compare
…rguments to distinct
joshuay03
force-pushed
the
raise-on-invalid-distinct-args
branch
from
9da87fb
to
84dee04
Compare
Motivation / Background
Fixes #51044
Detail
Updates the documented
ActiveRecord::QueryMethods#distinctand undocumentedActiveRecord::QueryMethods#distinct!methods toraise anlog a deprecation warning if given a non-boolean argument.ArgumentErrorAdditional information
None.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]