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

Add implicit receiver support to `Object#with_options` #16339

Merged
merged 1 commit into from Jul 29, 2014

Conversation

Projects
None yet
6 participants
@rwz
Contributor

rwz commented Jul 29, 2014

I've noticed that 99% of the cases I use Object#with_options to define a bunch of associations or validations in my models. The code could be much cleaner if we wouldn't have to pass explicit receiver to with_options block all the time, especially if it's gonna be the only receiver.

So, this pull request makes it possible. It's also fully backwards compatible and old syntax still works as expected.

Before:

class Account < ActiveRecord::Base
  with_options dependent: :destroy do |assoc|
    assoc.has_many :customers
    assoc.has_many :products
    assoc.has_many :invoices
    assoc.has_many :expenses
  end
end

After:

class Account < ActiveRecord::Base
  with_options dependent: :destroy do
    has_many :customers
    has_many :products
    has_many :invoices
    has_many :expenses
  end
end
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 29, 2014

Member

In my opinion this make the readability worse and my cause confusion where these call are being made. Depending on the code I don't know if the has_many contains that options or not. But it is just my opinion.

@tenderlove @jeremy @chancancode @dhh WDYT?

Member

rafaelfranca commented Jul 29, 2014

In my opinion this make the readability worse and my cause confusion where these call are being made. Depending on the code I don't know if the has_many contains that options or not. But it is just my opinion.

@tenderlove @jeremy @chancancode @dhh WDYT?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 29, 2014

Member

Ah, forgot to say, thank you for the pull request 😄

Member

rafaelfranca commented Jul 29, 2014

Ah, forgot to say, thank you for the pull request 😄

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 29, 2014

Member

I'm on the fence. I like the improved readability, but I'm generally very hesitant to use implicit receiver outside of narrowly defined DSLs, like routing. But on the other hand, how likely are you to have other calls inside the with_options call that doesn't want that receiver?

Rafael, do you have any code that does that?

Member

dhh commented Jul 29, 2014

I'm on the fence. I like the improved readability, but I'm generally very hesitant to use implicit receiver outside of narrowly defined DSLs, like routing. But on the other hand, how likely are you to have other calls inside the with_options call that doesn't want that receiver?

Rafael, do you have any code that does that?

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jul 29, 2014

Member

Yeah, I think instance_eval needs a pretty strong argument in its favour, against the confusion it can cause. And here, it just doesn't seem much of a gain.

IMO, the first example only looks ugly because of the large assoc... use a single-letter name, a la t in schema definitions, and it seems a more comfortable balance of minimal-yet-obvious.

Member

matthewd commented Jul 29, 2014

Yeah, I think instance_eval needs a pretty strong argument in its favour, against the confusion it can cause. And here, it just doesn't seem much of a gain.

IMO, the first example only looks ugly because of the large assoc... use a single-letter name, a la t in schema definitions, and it seems a more comfortable balance of minimal-yet-obvious.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jul 29, 2014

Member

Actually, doesn't this change the scope for any blocks defined therein? That seems bad.

Member

matthewd commented Jul 29, 2014

Actually, doesn't this change the scope for any blocks defined therein? That seems bad.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 29, 2014

Member

Just theoretic code. Depending on the size of the block the only thing that will say you if the call of the method use the option is the indentation.

Of course text editors will help with code highlight but say you have:

class Account < ActiveRecord::Base
  with_options dependent: :destroy do
    has_many :customers
    has_many :products
    has_many :invoices
    has_many :expenses
    has_many :cars
    has_many :books
    has_many :pages
    has_many :articles
  end

    has_many :users
    has_many :pirates
    has_many :ships
end

It is really hard to know what has applied options and what doesn't have.

Member

rafaelfranca commented Jul 29, 2014

Just theoretic code. Depending on the size of the block the only thing that will say you if the call of the method use the option is the indentation.

Of course text editors will help with code highlight but say you have:

class Account < ActiveRecord::Base
  with_options dependent: :destroy do
    has_many :customers
    has_many :products
    has_many :invoices
    has_many :expenses
    has_many :cars
    has_many :books
    has_many :pages
    has_many :articles
  end

    has_many :users
    has_many :pirates
    has_many :ships
end

It is really hard to know what has applied options and what doesn't have.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Jul 29, 2014

Contributor

@rafaelfranca the old syntax is still supported and nothing prevents you from using it. On the other hand, there are cases where implicit receiver syntax gives clear readability advantages. I as a developer prefer to have an option to use implicit/explicit rather than being forced to use explicit only.

Contributor

rwz commented Jul 29, 2014

@rafaelfranca the old syntax is still supported and nothing prevents you from using it. On the other hand, there are cases where implicit receiver syntax gives clear readability advantages. I as a developer prefer to have an option to use implicit/explicit rather than being forced to use explicit only.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 29, 2014

Member

Actually, I think that settles it for me. Since you can still use explicit, when you are mixing things that won’t just use implicit receiver, this covers it. I’m 👍.

On Jul 29, 2014, at 11:24, Pavel Pravosud notifications@github.com wrote:

@rafaelfranca the old syntax is still supported and nothing prevents you from using it. On the other hand, there are cases where implicit receiver syntax gives clear readability advantages. I as a developer prefer to have an option to use implicit/explicit rather than being forced to use explicit only.


Reply to this email directly or view it on GitHub.

Member

dhh commented Jul 29, 2014

Actually, I think that settles it for me. Since you can still use explicit, when you are mixing things that won’t just use implicit receiver, this covers it. I’m 👍.

On Jul 29, 2014, at 11:24, Pavel Pravosud notifications@github.com wrote:

@rafaelfranca the old syntax is still supported and nothing prevents you from using it. On the other hand, there are cases where implicit receiver syntax gives clear readability advantages. I as a developer prefer to have an option to use implicit/explicit rather than being forced to use explicit only.


Reply to this email directly or view it on GitHub.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 29, 2014

Member

👍 But @matthewd's comment about changing the block scope also worries me.

Member

rafaelfranca commented Jul 29, 2014

👍 But @matthewd's comment about changing the block scope also worries me.

@rondale-sc

This comment has been minimized.

Show comment
Hide comment
@rondale-sc

rondale-sc commented Jul 29, 2014

👍

Show outdated Hide outdated activesupport/lib/active_support/core_ext/object/with_options.rb
yield ActiveSupport::OptionMerger.new(self, options)
def with_options(options, &block)
merger = ActiveSupport::OptionMerger.new(self, options)
block.arity == 1 ? block.call(merger) : merger.instance_eval(&block)

This comment has been minimized.

@egilburg

egilburg Jul 29, 2014

Contributor

Even though we don't support more than 1 arity, under the principle of least surprise, I'd flip these around (so that if someone for whatever reason passes more than one, behavior won't change).

block.arity.zero? ? merger.instance_eval(&block) : block.call(merger)

@egilburg

egilburg Jul 29, 2014

Contributor

Even though we don't support more than 1 arity, under the principle of least surprise, I'd flip these around (so that if someone for whatever reason passes more than one, behavior won't change).

block.arity.zero? ? merger.instance_eval(&block) : block.call(merger)

This comment has been minimized.

@rwz

rwz Jul 29, 2014

Contributor

@egilburg good point, thanks.

@rwz

rwz Jul 29, 2014

Contributor

@egilburg good point, thanks.

dhh added a commit that referenced this pull request Jul 29, 2014

Merge pull request #16339 from rwz/with_options_implicit
Add implicit receiver support to `Object#with_options`

@dhh dhh merged commit c56997e into rails:master Jul 29, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment