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

Evaluate default procs in the context of the instance #217

Closed
mikekreeki opened this issue Aug 15, 2014 · 19 comments
Closed

Evaluate default procs in the context of the instance #217

mikekreeki opened this issue Aug 15, 2014 · 19 comments
Assignees
Milestone

Comments

@mikekreeki
Copy link

Virtus allows to use a result of a method call as a default value, like this for example:

class AgeCalculator
  include Virtus.model

  attribute :date_of_birth, Date, default: :parse_date

  attribute :year
  attribute :month
  attribute :day

  def execute
    # ...
  end

  private

  def parse_date
    Date.new(year, month, day)
  end
end

This is helpful, since this simple class can be initialized with both Date object or year/month/day triplet.

One could try to rewrite this class using ActiveIntraction the following way:

class AgeCalculator < ActiveInteraction::Base

  date :date_of_birth, default: -> { parse_date }

  integer :year, :month, :day

  def execute
    # ...
  end

  private

  def parse_date
    Date.new(year, month, day)
  end
end

ActiveInteraction allows to use procs as default values, which is helpful for values like Date.today, but the example above blows up:

NameError: undefined local variable or method `parse_date' for AgeCalculator:Class
  • Is it an intention that the proc is evaluated in the context of the class itself? Wouldn't it be more useful to lazily evaluate default procs in the context of an instance once attributes are assigned?
@tfausak
Copy link
Collaborator

tfausak commented Aug 15, 2014

It's intentional that the default values aren't evaluated in the instance's context. How would implicit dependencies between attributes be satisfied? For instance, if year defaulted to Date.today.year, would that be used in the calculation of parse_date?

class Example
  include Virtus.model
  attribute :date_of_birth, Date, default: :parse_date
  attribute :year, Integer, default: :default_year
  attribute :month
  attribute :day

  def execute; end

  private

  def parse_date
    Date.new(year, month, day)
  end

  def default_year
    Date.today.year
  end
end

If you run that and only give month and day, is year equal to nil or Date.today.year inside of parse_date?

Furthermore, since the individual components (year, month, and day) don't have defaults, they're required. Which means you have to provide them, even if you already provided date_of_birth. If you make them optional, which ones should take precedence?

@tfausak
Copy link
Collaborator

tfausak commented Aug 15, 2014

ActiveInteraction is capable of doing this. It's definitely verbose, but the release of v1.3 will help that (see #198).

class Example < ActiveInteraction::Base
  integer :year, :month, :day,
    default: nil
  date :birthday,
    default: nil

  # Calculate year, month, and day from birthday.
  set_callback :validate, :after, -> {
    return if ymd_valid? || !birthday_valid?

    self.year = birthday.year
    self.month = birthday.month
    self.day = birthday.day
    [:year, :month, :day].each { |k| errors.delete(k) }
  }

  # Calculate birthday from year, month, and day.
  set_callback :validate, :after, -> {
    return if birthday_valid? || !ymd_valid?

    self.birthday = Date.new(year, month, day)
    errors.delete(:birthday)
  }

  # Ensure the year, month, and day equal the birthday.
  set_callback :validate, :after, -> {
    return unless birthday_valid? && ymd_valid?
    return if birthday == Date.new(year, month, day)

    errors.add(:base, 'birthday must match year, month, and day')
  }

  # Ensure either the birthday or the year, month, and day were given.
  set_callback :validate, :after, -> {
    return if birthday_valid? || ymd_valid?

    errors.add(:base, 'either birthday or year, month, and day must be given')
  }

  def execute
    inputs
  end

  private

  def ymd_valid?
    year_valid? && month_valid? && day_valid?
  end

  def year_valid?
    year? && !errors.include?(:year)
  end

  def month_valid?
    month? && !errors.include?(:month)
  end

  def day_valid?
    day? && !errors.include?(:day)
  end

  def birthday_valid?
    birthday? && !errors.include?(:birthday)
  end
end

Example.run!
# ActiveInteraction::InvalidInteractionError: either birthday or year, month, and day must be given

Example.run!(year: 2001, month: 2, day: 3)
# => {:year=>2001, :month=>2, :day=>3, :birthday=>#<Date: 2001-02-03 ((2451944j,0s,0n),+0s,2299161j)>}

Example.run!(birthday: Date.new(2002, 3, 4))
# => {:year=>2002, :month=>3, :day=>4, :birthday=>#<Date: 2002-03-04 ((2452338j,0s,0n),+0s,2299161j)>}

Example.run!(year: 2001, month: 2, day: 3, birthday: Date.new(2002, 3, 4))
# ActiveInteraction::InvalidInteractionError: birthday must match year, month, and day

@tfausak tfausak added this to the v2.0.0 milestone Aug 15, 2014
@mikekreeki
Copy link
Author

Okey, thanks for the feedback! Of course, this would require user to think about the order of dependencies.

I guess parse_date would try to fetch year attribute, if not provided it would indeed return Date.today.year (of course day, month and year should be optional, my mistake). It should be said that this example actually don't work in Virtus either.

When provided with both date_of_birth and day, month, year triplet, I guess it should not be that smart to check if they actually match, that would be the responsibility of the user not to use both at the same time.

I'm not sure this example is a common use case for this pattern, though. I don't know, there may be better examples. It is just an idea to think about 😉

@tfausak
Copy link
Collaborator

tfausak commented Aug 18, 2014

Thanks for bringing this to our attention! Considering the number of edge cases that need to be considered to handle even this simple example, I don't expect us to add this functionality to ActiveInteraction. We will keep it in mind, though.

@tfausak
Copy link
Collaborator

tfausak commented Sep 3, 2014

@aaronjensen is also interested in this feature. He opened a couple of pull requests (#221 & #222) for it. I'm reopening this for discussion.

@tfausak tfausak reopened this Sep 3, 2014
@tfausak tfausak removed the wontfix label Sep 3, 2014
@tfausak
Copy link
Collaborator

tfausak commented Sep 3, 2014

My main problem with this feature is that you can accomplish the same thing with callbacks.

class A < ActiveInteraction::Base
  string :x, default: -> { self.class.name }
  def execute; inputs end
end
A.run! # => {:x=>"A"}
class B < ActiveInteraction::Base
  string :x, default: nil
  set_callback :type_check, :after, -> { self.x = self.class.name }
  def execute; inputs end
end
B.run! # => {:x=>"B"}

It's a little more complicated to use callbacks, but I think it's worth it; you have to be explicit about when you want them evaluated (before or after type checking or validation).

@aaronjensen
Copy link

Unfortunately callbacks only work when you're calling run or run!. If you're newing something up to use in a form helper they won't execute. I've been able to imitate this behavior by subclassing, though it is not w/o its warts: https://gist.github.com/aaronjensen/efa824496470b74f0fd7

@tfausak
Copy link
Collaborator

tfausak commented Oct 1, 2014

That an interesting use case to consider. The only officially supported way to deal with interactions is to call .run (or .run!). I don't want to add a feature to make an unsupported workflow easier. On the other hand, if it's an easy win, there's no sense in not doing it just because it's not the golden path.

Instead of using callbacks you could override the methods we define for you.

class C < ActiveInteraction::Base
  string :x, default: nil
  alias_method :_x, :x
  def x
    @x ||= self.class.name
    _x
  end
  def execute; inputs end
end
C.run! # => {:x=>"C"}

Unfortunately that's a lot of boilerplate. A helper method could handle most of it.

class ActiveInteraction::Base
  def self.default(attribute, &block)
    alias_method "_#{attribute}", attribute
    define_method attribute do
      unless instance_variable_get("@#{attribute}")
        instance_variable_set("@#{attribute}", instance_eval(&block))
      end
      send("_#{attribute}")
    end
  end
end

class D < ActiveInteraction::Base
  string :x, default: nil
  default(:x) { p self; self.class.name }
  def execute; inputs end
end
D.run! # => {:x=>"D"}

@aaronjensen
Copy link

Yeah that'd work. My method in that gist is a little bit more invasive/unstable to active_interaction changes. I'm ok w/ that though for the syntax elegance.

I'm surprised (though not that surprised) that .new isn't a "supported" thing, I suppose that goes back to the fact that you're primarily using it for apis whereas i'm trying to use it in a full stack rails app. I (wrongly, I guess) thought of AI as a form object library. We've been able to hack it into place so it works well enough for us, and I understand that you'd like to keep AI focused on only supporting run/run!. You may consider mentioning in the API that using it as a form object is not supported/recommended as it probably would have saved us some time (we would have stuck to virtus and a homegrown base class probably)

@tfausak
Copy link
Collaborator

tfausak commented Oct 1, 2014

ActiveInteraction definitely grew out of our need for managing business logic in an API. I would love for it to be more generally useful in Rails apps, but I think that goal is a ways off. We made it to be basically Mutations + ActiveModel; extending into form objects makes a lot of sense.

This change in particular can be implement in the backwards-compatible way (as I showed), meaning we could schedule it for 2.1 instead of 2.0. However if we wanted to support your way of doing things, I think we'd have to wait until 3.0.

Edit: And then we could make form objects (and Rails) the focus of 3.0.

@aaronjensen
Copy link

you may be able to do it in a backwards compatible way by checking the arity of the proc and passing the model in if the arity is 1: default: ->(m) { m.foo }

Either way, sounds good, thanks.

@tfausak
Copy link
Collaborator

tfausak commented Oct 2, 2014

Ooh, I like that idea.That's a clean way to differentiate between normal procs and ones that need to be evaluated in a special context.

string :x, default: 'normal string'
string :y, default: -> { 'lazy string' }
string :z, default: -> this { "#{this} string" }

The only downside is that they wouldn't be able to use private methods.

@AaronLasseigne
Copy link
Owner

I would disagree that .new is not a supported method. We use it in README examples and it is how our library works when used as a form object. The 1.2.0 release shows an effort to improve support for forms. I've successfully used it as a form object in some parts of the OrgSync application.

@tfausak
Copy link
Collaborator

tfausak commented Oct 2, 2014

Ah, right you are. That's my bad.

@tfausak
Copy link
Collaborator

tfausak commented Jul 30, 2015

I would like to add this feature, but it will require a significant refactoring of ActiveInteraction's internals. #290 had the right idea, but it didn't work for hash inputs. I think this feature should be part of v3.0.0. You could argue that it could be released with a minor version bump, but it is technically changing the API we provide.

@tfausak tfausak added this to the v3.0.0 milestone Jul 30, 2015
@tfausak tfausak removed their assignment Oct 26, 2015
@tfausak
Copy link
Collaborator

tfausak commented Oct 26, 2015

I still want to add this feature. But it's going to be hard to implement. If anyone wants to take a crack at it, #290 is a good start.

@tfausak
Copy link
Collaborator

tfausak commented Dec 15, 2015

This has the potential to interact strangely with hash defaults. Since hash defaults have to either be nil or {}, you couldn't really set a hash's default to the result of a proc. To wit:

hash :x, default: {} do
  integer :a, default: 0
end
# x defaults to { a: 0 }

# This will not work.
hash :y, default: -> { x } do
  integer :a
end

# This will work.
hash :z, default: {} do
  integer :a, -> { x[:a] }
end

That will quickly get annoying if you have a lot of fields in the hash.

This isn't a problem per se, but it's yet another oddity when it comes to default values for hashes.

@tfausak
Copy link
Collaborator

tfausak commented Dec 16, 2015

Even though this is on the v3.0.0 milestone, I think it could be added to v2.2.0. We do not guarantee which binding will be used when evaluating defaults. In spite of that, it is always nil, which is worthless. The only way the changes in #332 could break anything is if you relied on the default value throwing an error. That's not a reasonable use case.

tfausak added a commit that referenced this issue Dec 18, 2015
Evaluate default procs with the interaction's binding
@tfausak
Copy link
Collaborator

tfausak commented Dec 18, 2015

Y'all should be happy to hear that this feature has been released as part of ActiveInteraction v2.2.0. Sorry it took so long 😄

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