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

Pass block to Relation#distinct and Relation#uniq #20198

Closed
wants to merge 3 commits into from

Conversation

@hwhelchel
Copy link

hwhelchel commented May 19, 2015

Observes Array#uniq semantics when passing a block to distinct or uniq. Loads an array of the relation and then uses the return value of the block to determine uniqueness.

See: #20189

@hwhelchel

This comment has been minimized.

Copy link
Author

hwhelchel commented May 19, 2015

Looks like Relation#distinct started out as Relation#uniq. Because it didn't behave exactly like Array#uniq there was confusion so the method was renamed to distinct which reflects the existing SQL behavior.

Today, uniq and uniq! are marked as silently deprecated and are aliases of distinct and distinct! respectively. I therefore updated distinct and emulated how the select method handles blocks.

@senny

This comment has been minimized.

Copy link
Member

senny commented May 19, 2015

As you pointed out, we renamed uniq back to distinct. The Relation object is used to build SQL queries, I'd rather not add behavior that acts on the returned Ruby objects. If it's not something we can do on the database end I prefer to use the result array directly:

relation.to_a.uniq {}

This clearly communicates that the queries is executed before the applying the uniqueness constraint.

I'm 👎 on this addition but let's get some more feedback before deciding.

/cc @rafaelfranca @carlosantoniodasilva @sgrif

@senny senny added the activerecord label May 19, 2015
@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented May 19, 2015

I think I'd rather leave the block out of this, and let users call to_a when they actually need an array back to work with. So I agree with @senny. Thanks for your contribution.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented May 21, 2015

Today, uniq and uniq! are marked as silently deprecated and are aliases of distinct and distinct! respectively.

If it's not something we can do on the database end I prefer to use the result array directly:

So lets deprecate uniq or raise an argument error when block is passed. Right now it is hard to tell if we are using array method or relation method.

@hwhelchel

This comment has been minimized.

Copy link
Author

hwhelchel commented May 22, 2015

@rafaelfranca 's recommendation is in line with the history of changes to this method. I think deprecating uniq would be best. Let me know if you all agree.

@senny

This comment has been minimized.

Copy link
Member

senny commented May 26, 2015

@hwhelchel yup, seems reasonable. I introduced distinct as a clear successor to uniq but since uniq was pretty new I didn't hard deprecate it at the time.

I'll add in the Relation#uniq deprecations on master for Rails 5.

senny added a commit that referenced this pull request May 26, 2015
See #9683 for the reasons we switched to `distinct`.

Here is the discussion that triggered the actual deprecation #20198.

`uniq`, `uniq!` and `uniq_value` are still around.
They will be removed in the next minor release after Rails 5.
@senny

This comment has been minimized.

Copy link
Member

senny commented May 26, 2015

@hwhelchel @rafaelfranca Added the discussed deprecation adfab2d

Going to close this PR. Thanks for reporting 💛

@senny senny closed this May 26, 2015
pikender pushed a commit to vinsol-spree-contrib/spree_favorite_products that referenced this pull request Feb 8, 2017
Fixed deprecation warning uniq -> distinct
See: rails/rails#20198
Fixed exception for validate_presence “The record was indeed invalid, but it produced these validation errors instead”
See: thoughtbot/shoulda-matchers#861 (comment)
Fixed deprecation warning - use keyword syntax for get, put etc in controller tests
Fixed coffee-rails not loading error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.