Added ActiveRecord::Relation#none! method #7750

Merged
merged 2 commits into from Oct 28, 2012

Conversation

Projects
None yet
7 participants
@xuanxu
Contributor

xuanxu commented Sep 24, 2012

The none bang method in AR::Relation to modify objects in place was missing.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Sep 24, 2012

Member
Member

fxn commented Sep 24, 2012

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Sep 24, 2012

Contributor

👎 we should not promote mutating relations in place.

Contributor

josevalim commented Sep 24, 2012

👎 we should not promote mutating relations in place.

@xuanxu

This comment has been minimized.

Show comment Hide comment
@xuanxu

xuanxu Sep 24, 2012

Contributor

@josevalim ¿? but every other query method has already a paired method to modify relations in place (from!, where! select!, extending!, having!, order!....)

Contributor

xuanxu commented Sep 24, 2012

@josevalim ¿? but every other query method has already a paired method to modify relations in place (from!, where! select!, extending!, having!, order!....)

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Oct 26, 2012

Contributor

Can you rebase it?

Contributor

frodsan commented Oct 26, 2012

Can you rebase it?

@xuanxu

This comment has been minimized.

Show comment Hide comment
@xuanxu

xuanxu Oct 26, 2012

Contributor

Rebased

Contributor

xuanxu commented Oct 26, 2012

Rebased

@jonleighton

This comment has been minimized.

Show comment Hide comment
@jonleighton

jonleighton Oct 26, 2012

Member

@josevalim I agree we shouldn't really promote it. I added mutation mainly for internal use plus for optimisation of bottleneck areas in app code. But we should probably add this simply for consistency, wdyt?

Member

jonleighton commented Oct 26, 2012

@josevalim I agree we shouldn't really promote it. I added mutation mainly for internal use plus for optimisation of bottleneck areas in app code. But we should probably add this simply for consistency, wdyt?

@frodsan

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Oct 26, 2012

Contributor

👍 for consistency. If we shouldn't really promote it, why we have them in the public API? Maybe we can :nodoc: them.

Contributor

frodsan commented Oct 26, 2012

👍 for consistency. If we shouldn't really promote it, why we have them in the public API? Maybe we can :nodoc: them.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 26, 2012

Member

👍 for consistency

Member

rafaelfranca commented Oct 26, 2012

👍 for consistency

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2012

Member

I'm also 👍 for consistency. @xuanxu github says it cannot be cleanly merged anymore, probably the changelog, can you rebase it please?

I'm also 👍 for consistency. @xuanxu github says it cannot be cleanly merged anymore, probably the changelog, can you rebase it please?

@xuanxu

This comment has been minimized.

Show comment Hide comment
@xuanxu

xuanxu Oct 28, 2012

Contributor

@carlosantoniodasilva yep, it was the changelog. Rebased now.

Contributor

xuanxu commented Oct 28, 2012

@carlosantoniodasilva yep, it was the changelog. Rebased now.

rafaelfranca added a commit that referenced this pull request Oct 28, 2012

@rafaelfranca rafaelfranca merged commit 852e376 into rails:master Oct 28, 2012

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 28, 2012

Member

Thank you

Member

rafaelfranca commented Oct 28, 2012

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment