rename Relation#uniq to Relation#distinct

[senny]

1.

The similarity of Relation#uniq to Array#uniq is confusing. Since our
Relation API is close to SQL terms I renamed #uniq to #distinct.

There is no deprecation. #uniq and #uniq! are aliases and will continue
to work. I also updated the documentation to promote the use of #distinct.
c7f94f0

2.

Deprecate the :distinct option for Relation#count.
We moved more and more away from passing options to finder / calculation
methods. The :distinct option in #count was one of the remaining places.
Since we can now combine Relation#distinct with Relation#count the option
is no longer necessary and can be deprecated.

[senny]

@jonleighton @carlosantoniodasilva what do you think?

[carlosantoniodasilva]

Probably this doc should be also updated.

[jonleighton]

@senny ^^ ping about this

[senny]

missed this one 😕 . Pushed an updated version.

[carlosantoniodasilva]

Overall it looks good, just afraid that people could be using uniq_value directly, and that could break?

[senny]

@carlosantoniodasilva we could alias it too...

[jonleighton]

Seems good to me. I agree about aliasing uniq_values. We should probably explicitly mention that uniq is "soft deprecated" and may be switched back to doing to_a.uniq in the future, so new code should definitely be written using distinct.

[senny]

@jonleighton I alias #uniq_value and described the silent deprecation.

[carlosantoniodasilva]

👍 ❤️

[bughit]

this seems wrong, distinct is a parameter specifically to the count expression COUNT(DISTINCT date(attr)), so it's completely logical for it to be a parameter to the AR analogue of COUNT, the count method. Separating distinct from count does not make sense.

.count('date(attr)', distinct: true)

is this supposed to be a replacement for the above?

.distinct. ...possible intervening chain... .count('date(attr)')

this both reads wrong, and produces unintended (albeit innocuous in this case) sql:

SELECT DISTINCT COUNT(DISTINCT date(created_at))

[rafaelfranca]

.count('DISTINCT date(attr)') should work fine.

[bughit]

so would .count_by_sql, but why is it deemed necessary to break count('expression', distinct: true)

[rafaelfranca]

The reason is to make the API consistent with our view of not passing options to finders/calculations methods. It is a matter of reducing the API possibilities to focus on what is important.

This feature in my opinion doesn't add anything over .count('DISTINCT date(attr)'), so we don't need to keep it.

[bughit]

This feature in my opinion doesn't add anything ...

That may be an argument for rejecting a feature request, but not for introducing a breaking change.
We are talking about one line of code: distinct = options[:distinct]
It and the code that relies on it .count('expression', distinct: true) are not so offensive that a breaking change is needed.

[senny]

In my opinion keeping the API consistent is worth a breaking change. There were also special cases where distinct: true did not work. For example PostgreSQL allows you to do a distinct on multiple columns:

SELECT COUNT(DISTINCT(name, email)) FROM members;

It's better to let the user know that he can specify the exact count clause than abstracting that away with the distunct: true option.

Member.count("DISTINCT(name, email)")

Also the Relation#distinct method is there to be combined with `select´:

Person.select("name").distinct.count

Those two avenues provide a more complete API and there is no more need to keep the third option (passing distinct: true) around.

[rafaelfranca]

That may be an argument for rejecting a feature request, but not for introducing a breaking change.

Yes, it is an argument to introducing a breaking change too for two reasons:

1. Rails Core don't want to maintain this feature in the future. The behavior introduced by this feature is not worth the maintenance cost or the doubts that it can introduce for the users like the examples @senny added in the last comment.
2. Rails 4 was a major release so breaking changes are expected and it was the best moment to remove code that doesn't fit the direction the framework is taking or doesn't seems a good API.