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

Fix behavior of select! to be consistent with select #14752 #14757

Merged
merged 1 commit into from Apr 22, 2014

Conversation

Projects
None yet
5 participants
@estsauver
Contributor

estsauver commented Apr 15, 2014

Fixes #14752

Select mimics the block interface of arrays, but does not mock the
block interface for select!. This change introduces two changes to
the behavior of select!.

  1. if select! is called without an argument or a block, an error
    is raised (consistent with select)

  2. if select! is called with a block, it delegates the call to the
    array representation of the association.

Thanks to @eric-smartlove for pointing out the issue.

@rafaelfranca

View changes

Show outdated Hide outdated activerecord/lib/active_record/relation/query_methods.rb Outdated
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 15, 2014

Member

Is select! the only problem? Could you check with the others bang methods on this module?

Member

rafaelfranca commented Apr 15, 2014

Is select! the only problem? Could you check with the others bang methods on this module?

@rafaelfranca

View changes

Show outdated Hide outdated activerecord/lib/active_record/relation/query_methods.rb Outdated
@rafaelfranca

View changes

Show outdated Hide outdated activerecord/test/cases/relations_test.rb Outdated

@rafaelfranca rafaelfranca added this to the 4.0.6 milestone Apr 15, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 15, 2014

Member

Ok. We decided what will be the path to take.

  1. Rename select! to _select!
  2. Add select! to this blacklist
    BLACKLISTED_ARRAY_METHODS = [

For 4-0-stable we'll need another PR with the current implementation here.

@estsauver WDYT?

Member

rafaelfranca commented Apr 15, 2014

Ok. We decided what will be the path to take.

  1. Rename select! to _select!
  2. Add select! to this blacklist
    BLACKLISTED_ARRAY_METHODS = [

For 4-0-stable we'll need another PR with the current implementation here.

@estsauver WDYT?

@estsauver

This comment has been minimized.

Show comment
Hide comment
@estsauver

estsauver Apr 15, 2014

Contributor

That seems great. I actually didn't know we had that blacklist of methods.

I'll put in a PR tonight.

~Earl

On Tue, Apr 15, 2014 at 10:25 AM, Rafael Mendonça França <
notifications@github.com> wrote:

Ok. We decided what will be the path to take.

  1. Rename select! to _select!
  2. Add select! to this blacklist
    BLACKLISTED_ARRAY_METHODS = [

For 4-0-stable we'll need another PR with the current implementation here.

@estsauver https://github.com/estsauver WDYT?


Reply to this email directly or view it on GitHubhttps://github.com//pull/14757#issuecomment-40509509
.

Contributor

estsauver commented Apr 15, 2014

That seems great. I actually didn't know we had that blacklist of methods.

I'll put in a PR tonight.

~Earl

On Tue, Apr 15, 2014 at 10:25 AM, Rafael Mendonça França <
notifications@github.com> wrote:

Ok. We decided what will be the path to take.

  1. Rename select! to _select!
  2. Add select! to this blacklist
    BLACKLISTED_ARRAY_METHODS = [

For 4-0-stable we'll need another PR with the current implementation here.

@estsauver https://github.com/estsauver WDYT?


Reply to this email directly or view it on GitHubhttps://github.com//pull/14757#issuecomment-40509509
.

@eric-smartlove

This comment has been minimized.

Show comment
Hide comment
@eric-smartlove

eric-smartlove Apr 15, 2014

@rafaelfranca This solution seems perfect to me: it hides a method that is currently like an open trap for the developer. 👍

eric-smartlove commented Apr 15, 2014

@rafaelfranca This solution seems perfect to me: it hides a method that is currently like an open trap for the developer. 👍

@laurocaetano

This comment has been minimized.

Show comment
Hide comment
@laurocaetano

laurocaetano Apr 15, 2014

Contributor

@estsauver you are going have problems here and here, because it will call send("select!",...).

The easy way to fix it is just checking if the method is :select and then call send("_select!", ...). It could be something like this.

Btw, it is not the ideal implementation (we could rename all these internals mutator methods to use underscore), but that's another story. EDIT: It would be good to try it.

Contributor

laurocaetano commented Apr 15, 2014

@estsauver you are going have problems here and here, because it will call send("select!",...).

The easy way to fix it is just checking if the method is :select and then call send("_select!", ...). It could be something like this.

Btw, it is not the ideal implementation (we could rename all these internals mutator methods to use underscore), but that's another story. EDIT: It would be good to try it.

@estsauver

This comment has been minimized.

Show comment
Hide comment
@estsauver

estsauver Apr 17, 2014

Contributor

Pull Request is updated: Comments welcomed.

Contributor

estsauver commented Apr 17, 2014

Pull Request is updated: Comments welcomed.

@laurocaetano

View changes

Show outdated Hide outdated activerecord/lib/active_record/relation/merger.rb Outdated
@laurocaetano

View changes

Show outdated Hide outdated activerecord/test/cases/relations_test.rb Outdated
@matthewd

View changes

Show outdated Hide outdated activerecord/lib/active_record/relation/merger.rb Outdated
@laurocaetano

View changes

Show outdated Hide outdated activerecord/test/cases/relation/mutation_test.rb Outdated

@laurocaetano laurocaetano modified the milestones: 4.1.1, 4.0.6 Apr 21, 2014

@estsauver

This comment has been minimized.

Show comment
Hide comment
@estsauver

estsauver Apr 21, 2014

Contributor

Changes made.

Contributor

estsauver commented Apr 21, 2014

Changes made.

@laurocaetano

This comment has been minimized.

Show comment
Hide comment
@laurocaetano

laurocaetano Apr 21, 2014

Contributor

@estsauver looks good. Nice job ❤️

I think we should also update the commit message, wdyt?

Make select! a private method _select! #14752 … (it is not a private method, it's just private API.)
Fixes #14752

Select mimics the block interface of arrays, but does not mock the
block interface for select!. This change moves the api to be a
private method, _select!. 

It also raises an error if a block is given, which should
make sure it's not mistaken for the array select! (we are not raising an error when a block is passed)

Maybe just saying that select! was renamed to _select! to avoid confusions with Array#select! is good to go.

Contributor

laurocaetano commented Apr 21, 2014

@estsauver looks good. Nice job ❤️

I think we should also update the commit message, wdyt?

Make select! a private method _select! #14752 … (it is not a private method, it's just private API.)
Fixes #14752

Select mimics the block interface of arrays, but does not mock the
block interface for select!. This change moves the api to be a
private method, _select!. 

It also raises an error if a block is given, which should
make sure it's not mistaken for the array select! (we are not raising an error when a block is passed)

Maybe just saying that select! was renamed to _select! to avoid confusions with Array#select! is good to go.

@estsauver

This comment has been minimized.

Show comment
Hide comment
@estsauver

estsauver Apr 21, 2014

Contributor

Good call, I'll update the commit message.
On Apr 21, 2014 11:47 AM, "Lauro Caetano" notifications@github.com wrote:

@estsauver https://github.com/estsauver looks good. Nice job [image:
❤️]

I think we should also update the commit message, wdyt?

Make select! a private method _select! #14752 … (it is not a private method, it's just private API.)
Fixes #14752

Select mimics the block interface of arrays, but does not mock the
block interface for select!. This change moves the api to be a
private method, _select!.

It also raises an error if a block is given, which should
make sure it's not mistaken for the array select! (we are not raising an error when a block is passed)

Maybe just saying that select! was renamed to _select! to avoid
confusions with Array#select! is good to go.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14757#issuecomment-40963276
.

Contributor

estsauver commented Apr 21, 2014

Good call, I'll update the commit message.
On Apr 21, 2014 11:47 AM, "Lauro Caetano" notifications@github.com wrote:

@estsauver https://github.com/estsauver looks good. Nice job [image:
❤️]

I think we should also update the commit message, wdyt?

Make select! a private method _select! #14752 … (it is not a private method, it's just private API.)
Fixes #14752

Select mimics the block interface of arrays, but does not mock the
block interface for select!. This change moves the api to be a
private method, _select!.

It also raises an error if a block is given, which should
make sure it's not mistaken for the array select! (we are not raising an error when a block is passed)

Maybe just saying that select! was renamed to _select! to avoid
confusions with Array#select! is good to go.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14757#issuecomment-40963276
.

select! renamed to avoid name collision Array#select!
Fixes #14752

Select mimics the block interface of arrays, but does not mock the
block interface for select!. This change moves the api to be a
private method, _select!.
@laurocaetano

This comment has been minimized.

Show comment
Hide comment
@laurocaetano

laurocaetano Apr 22, 2014

Contributor

looks good!

@rafaelfranca :shipit: ?

Contributor

laurocaetano commented Apr 22, 2014

looks good!

@rafaelfranca :shipit: ?

rafaelfranca added a commit that referenced this pull request Apr 22, 2014

Merge pull request #14757 from estsauver/14752
Fix behavior of select! to be consistent with select #14752

@rafaelfranca rafaelfranca merged commit ba84bd9 into rails:master Apr 22, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Apr 22, 2014

Merge pull request #14757 from estsauver/14752
Fix behavior of select! to be consistent with select #14752

rafaelfranca pushed a commit that referenced this pull request Apr 22, 2014

rafaelfranca added a commit that referenced this pull request Apr 23, 2014

Merge pull request #14757 from estsauver/14752
Fix behavior of select! to be consistent with select #14752

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/relation/delegation.rb
	activerecord/lib/active_record/relation/query_methods.rb
	activerecord/test/cases/relation/mutation_test.rb

tenderlove added a commit that referenced this pull request Apr 25, 2014

Merge branch 'master' into adequaterecord
* master: (28 commits)
  move AR length validation tests into separate test-case.
  No need for trailing slash on migration path.
  reset `@arel` when modifying a Relation in place.
  PostgreSQL Timestamps always map to `:datetime`.
  [ci skip] Improve formatting and yml
  Fix a typo in the doc of forty_two AR FinderMethod
  Improve readability of contributing to rails guide. [ci skip]
  Precompile the image we're referencing, too.
  `ActiveRecord::Base.no_touching` no longer triggers callbacks or start empty transactions.
  Fixed an issue with migrating legacy json cookies.
  Correct comment [ci skip]
  Perfer to define methods instead of calling test
  Fix syntax error
  Add CHANGELOG entry for #14757 [ci skip]
  Fix run-on sentences and improve grammar [skip ci]
  Add test for using ActionView::Helpers::FormHelper.label with block and html
  select! renamed to avoid name collision Array#select!
  Rearrange deck chairs on the titanic. Organize connection handling test cases.
  Change favicon_link_tag helper mimetype from image/vnd.microsoft.icon to image/x-icon.
  ActionController::Renderers documentation fix
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment