Added a contract DSL for interactors

[jonstokes]

Brian and I have been talking about doing this and using it to clean up the eat app, so I hacked this together and wanted to put it out there for you guys to look at.

This current PR breaks backwards compatibility, but if we wanted to not do that then we could require the user to include Interactor::Contract to make it work.

A few notes on the DSL:

class BuildContact
    include Interactor

    expects :email, :first_name

    allows :last_name
    allows(:category) { ContactCategory.default }
    allows(:tags, :labels) { [] }
    allows(:phone_number)

    provides :full_name do
      next first_name unless last_name
      "#{first_name}, #{last_name}"
    end

    def call
      self.tags << "Collective Idea"
      puts full_name # "Doe, John"

      self.full_name = "Doe, John B."
      puts full_name # "Doe, John B."
    end
  end

Given the above, you can expect the following behaviors:

  context = BuildContact.call(
    email: "client@clientsite.com",
    first_name: "John",
    last_name: "Doe"
  )
  context.category == :business
  context.tags == ["Collective Idea"]
  context.labels == []
  context.phone_number == nil
  context.full_name == "Doe, John B."

And

  BuildContact.call(
    email: 'foo@bar.com',
    favorite: true,
    full_name: "Doe, John"
  )
  #=> Interactor::ContractError: Expected property 'first_name' not to be nil. Undeclared properties 'favorite', 'full_name'. 

There's actually another, more general DSL lurking under the hood here, in case we want to include some other types of options and/or validations in the future. So the above interactor could be written as follows and it would work the same way (because the version above is just sugar for the version below):

  class BuildContact
    include Interactor

    property :email,        presence: :required

    property :last_name,    presence: :permitted
    property(:category,     presence: :permitted) { ContactCategory.default }
    property(:tags,         presence: :permitted) { [] }
    property(:labels,       presence: :permitted) { [] }
    property :phone_number, presence: :permitted

    property(:full_name,    presence: :provided) do
      next first_name unless last_name
      "#{last_name}, #{first_name}"
    end

    def call
      self.tags << "Collective Idea"
      puts full_name
    end
  end

Anyway, I didn't want to take this any further until I got some feedback on it. Fire away!

[jonstokes]

From the Travis failure, it looks like this only works with Ruby 2.0+. I don't know if that's a bug or a feature.

[laserlemon]

Thanks for taking the time to hack this together! :clap:

Right off the bat, I'll say that I think this or something like it would be a good candidate for a version 4.0. There are some other changes that need to be made to interactor re: hooks and those will also be backwards compatible.

Ruby 2.0 only is a feature in my mind. The next version should drop Ruby 1.9 support. Dropping 1.9 will also let us use Module#prepend which I'm pretty happy about. It'll let us call the call method on an instance and still magically get all of the hooks.

Anyway, I'll have more feedback specific to these changes soon!

[laserlemon]

I love the idea of being more explicit with what each interactor gets and sets and I think that it's high time that Interactor include something like these changes. I have some questions as well as some concerns with this particular design:

Questions

1. Do you think there's room for these kinds of macro methods (expects, allows, provides, etc.) without adding direct access to the context's properties from the interactor instance?

2. When would an interactor's expectations be evaluated? Before or after hooks are run? I'm specifically thinking about an interactor that requires a user in the context but can either be given a user or a user_id. What would that look like?

   class DoSomething
   include Interactor
   
   expects :user { User.find(user_id) }
   allows :user_id
   end

3. Is the next in your example supposed to be a return? If not, I'm confused.
4. Is your custom error raised if the value for a required property is nil or if it isn't set at all?

Concerns

1. The design/naming doesn't feel quite obvious enough to me yet. Unfortunately, I say that without suggestion for improvement yet.
2. I'm unsure of using the macro method's block for the default value. It would be clearer in my mind to set a :default option, perhaps as a proc if it needs to be evaluated at runtime. That introduces another issue though if you want to actually use a proc as the default value. :neutral_face:
3. I don't like direct access to context properties from the interactor instance. I think it makes it unclear as to where these properties actually live. It's especially confusing for me to see self.foo = "bar". The self. despite not being set on self really drives the point home to me that something ain't quite right. More on that discussion in #47.
4. Raising an error when an unexpected property is present in the context doesn't feel useful to me. When being called within an organizer, there's no guarantee that the context passed to an individual interactor will have only the properties it's expecting.

That's all I have at the moment. I'd love to get other :eyes: on this as well.

[jonstokes]

Steve, thanks for the prompt feedback!

Questions

1) I'm not sure I follow on this one. Can you give an example of what you're thinking of? My guess is that you'd rather see expects, allows, and provides wrap a set of instance variables that at the outset are populated by the context and on exit dump all their updated values into the context, so that the context acts as a sort of transport but it's not directly manipulated within the interactor. Is this correct?

2) The expectations are always validated before any hooks are run. And then at the end, after all hooks are run, any defaults that weren't overridden in call or in the hooks are set using the provided blocks.

As for the example you gave, you'd probably have to write it this way:

class DoSomething
  include Interactor

  allows :user { User.find(user_id) rescue nil }
  allows :user_id

  def call
    raise "Requires user or user_id" unless user
  end
end

In the above code, if you call user first in call then it'll try to execute the block, which means that if you didn't pass a user_id as part of the context then it'll be doing User.find(nil).

I understand that this isn't quite what you want. You probably want something that looks like this:

expects or: :user_id, :user
allows(:user) { User.find(user_id }

I decided to keep the DSL pretty lean, so I didn't support anything like an or: clause. Instead, if you want to get fancy with the combinations of what it expects or doesn't expect, you should just use allows and handle the validation from within call or (even better) a before hook.

3) The next works like a return from within the block, and just leaves the block context. On jruby, at least, sticking a return there would throw an error. I should probably support the ability to do something like the following, so you can just use a return as normal:

allows :full_name, default: :generate_full_name

def generate_full_name
 return first_name unless last_name
  "#{last_name}, #{first_name}"
end

4) Good catch. The version I originally pushed expected the properties to be non-nil, but I just changed it to only expect them to be present -- nil is now a valid value for a property, as long as its key is there in the context when the interaction is called. The side effect of this change is that if you stick a block after expects that can now get called to override a nil value that you pass in (this wouldn't have worked before, because it would have errored before ever getting to the block).

Concerns

1) I struggled with this a lot over the weekend, and ultimately decided that "perfect is the enemy of good" :) I'm all ears for improvements, though. And incidentally, this is why I ended up making two DSLs: one verbose and one less expressive that's sugar for the verbose one. The verbose one could be extended easily in a number of different directions, and it's pretty trivial write your own Interactor::MyContractDSL and include it that implements something you like better on top of the more verbose property(:property_name, presence: presence_type) { initializer_block } DSL.

2) I personally like the block, but as I said above I think I should include a default: :method_name option.

For my own sanity, at least, I'd prefer to stick to either a block or a method name for default:, and not pass an object. The reason is that if you did something like allow :property, default: some_other_property then later on in a hook or in the call method you might start mutating some_other_property but then you're also mutating property, as well. Did you want to do that, or did you mean to set default: some_other_property.dup or even default: some_other_propery.deep_dup (if it's a hash). Not that you couldn't get into this kind of trouble with the block format, but at least it feels a little safer and more explicit.

3) Ultimately, the calls to expects, allows, and provides are intended to feel like calls to attr_accessor in a PORO. You put those up top, then you use the getters and setters exactly like you would in a PORO with attr_accessor, except that instead of getting and setting @property you're getting and setting context[property].

Of course, as you point out, context isn't defined on the instance so the use of self for the setters is misleading if you're thinking of it as working exactly like attr_accessor under the hood. But it's supposed to feel like attr_accessor without actually working that way.

I guess I could take a stab at having the context object go totally unmutated within the actual body of the interactor (i.e. hooks + call), so that the contract's property declarations really do create instance variables that the interactor manipulates before writing them out to a context object on exit after the last hook is run.

4) Yep, I get it, because you may have some organizers that re-use simple interactors where this would force you to stuff a bunch of scenario-specific property declarations into them to cover every conceivable use case (or some other contortion involve a property that's just a hash so that you can slip in whatever you want). I'll remove this behavior.

[michaelherold]

Concerns

My first thought about this was that the contract violations won't play nice with the #fail! method and the Failure capture in Interactor.run. I think ContractError should be a type of Failure so that rollbacks can happen if a contract isn't met. Also, ContractViolation would be a better name in my opinion -- or even partitioning that into PreconditionViolation and PostconditionViolation to give even more information about when a failure occurs.

Secondly, it would be great to have configurable behavior for what happens when a contract is violated. For example, in an app I'm working on I have the convention of all Interactor failures failing with a fail!(message: description_of_problem) that is then presented to the user. I use the i18n framework with codified messages that are then translated from the language files; hence, it's very important to me to be able to control the content of Context#message in the Interactor itself. As this PR exists, I will have to find a place to wrap the message in from a failure condition.

Suggestion

I break my business logic down into the smallest possible bits and then organize the steps. A great feature to have relating to this feature would be a walk of the contracts in an organizer's chain of interactors to make sure that the whole chain of contracts makes sense. For example:

class SetupAPICall
  include Interactor

  expects :user
  provides :api_key
end

class CreateIdempotencyKey
  include Interactor

  provides :idempotency_key
end

class MakeAPICall
  include Interactor

  expects :api_key, :idempotency_key
  provides :result
end

class MakeIdempotentAPICall
  include Interactor::Organizer

  organizes SetupAPICall,
            CreateIdempotencyKey,
            MakeAPICall
end

The "inferred" contract for MakeIdempotentAPICall would be: expects :user, provides :result, while the api_key and idempotency_key are intermediate values that are needed for those individual steps.

When creating this business logic chain, if we had the ability to walk the contract chain of the organizer, it would be immediately obvious if we, for instance, forgot to include the CreateIdempotencyKey interactor in the chain because the MakeAPICall contract wouldn't be able to be met by the SetupAPICall interactor.

This type of thing, along with the boilerplate that the contracts inherently can eliminate are the major pain points for me when using Interactor.

[jonstokes]

@michaelherold I thought about making contract violations fail the context, but I ultimately decided to raise an error because of the nature of what the contract is.

As far as an interactor's external interface goes, expects and allows part of the contract is intended to be like keyword arguments but for an entire interactor -- some args are required, some are optional, and some are optional with defaults -- which means that the contract is statically defined and throws an error at runtime if you try and call its interactor with the wrong arguments. (Internally to the interactor these end up working more like attr_accessor).

To my mind, a facility for failing the context if certain expectations about the context aren't met would be a great addition, and in fact I actually started hacking that (by adding abort_if { block } and abort_unless { block } methods) before deciding to limit this PR specifically to the "just like method arguments, but for interactiors" idea.

(Obviously, provides does not fit the method args paradigm in ruby, but that's only because ruby isn't a language that has you declare anything about a method's return value up front. So it may not be a pattern that's native to ruby, but it's common enough that it still fits generically under the "this is like the interface to a method, and if you violate it you raise an exception" paradigm.)

[jonstokes]

What about the addition of something like the following:

on_violation(:property_name) do
  puts "Called instead of raising error if context[:property] violates contract. You can fail the context here yourself, if you like."
  puts "If there's no on_violation call for a particular property, an error gets raised instead."
end

If this facility were present, then it would be trivial to override expects and allows to support failing the context instead of throwing an error but without having to actually write an on_violation block each time. That way, if you wanted to automatically fail the context instead of throwing an error, just include your own module that does this.

[michaelherold]

I'll think about your longer response before replying to it, but I do have some immediate thoughts on your on_violation proposal. I think being able to know a priori what property has a violation and what violation it is would be useful. So something like this would be preferable to me:

on_violation do |property, violation|
  puts "#{property} violated the contract by: #{violation}"
end

Whereas property would be the property name (symbol or string?) and violation could either be a convention-following symbol (i.e. :missing, :invalid, or whatever we want to define as the convention) or a Violation class that describes what violation occurred. I think currently the only violation would be a missing parameter, but that could be extended later for type coercion and a number of other things.

I really like the idea of on_violation. We could provide a default in the Contract mixin (which is currently your raise), but allow the user to have their own application-level default defined to override the gem-level default. I think it would add a lot of flexibility without a lot of code or complexity in the gem.

[michaelherold]

@jonstokes Regarding your longer comment, I understand your view of expects and allows: you're seeing them as an "interactor signature". I think that's perfectly acceptable when using interactors in isolation. However, there are two issues that I think are pertinent:

1. Interactors are most useful, in my opinion, when using them leads to a brevity in your code. There's just something satisfying in following the simple pattern of using interactors. For example:

   class MyController < ActionController::Base
    def do_something
      interaction = MyInteractor.call(interactor_params)
   
      if interaction.success?
        # Do success action
      else
        # Do failure action
      end
    end
   end

   This simplicity makes it very easy to grok what's going on in this controller action. And the more you invest in interactors, the more value you find in them, since all of the setup around your business logic looks and feels the same. However, if we raise an exception on contract violation, the user has to throw in ugly, harder-to-reason-about rescue statement, which feels to me like "control flow by exception."

2. An organizer (which, you may have gathered, I make heavy use of) would need to know what to do to handle a raised exception or, alternatively, forego the benefits of rollbacks and leave its workflow in an inconsistent state when an interactor in the middle of its chain violates its contract. This could be mitigated somewhat if we can implement the "walk the chain of contracts" behavior I suggested earlier by validating that the chain can successfully meet all the contracts of its organizers. I'm unsure if it can be mitigated entirely ... I've been wrestling with that in my head for a while.

I'll think some more on the issue, but I did want to voice my thoughts. I actually started working on a similar pull request last week, but hadn't made much progress yet! Kudos to being a first mover on the issue! :+1:

[jonstokes]

@michaelherold I'm pretty sold on the idea of offering both options via on_violation { } or something like it. So yeah, let's support both in some way that lets users easily pick.

One question I have, though, is about how the following would work:

on_violation do |property, violation|
  puts "#{property} violated the contract by: #{violation}"
end

Can you give a short code example for some context of how this would be used? I think I get it, but I want to be sure, because the way I'm imagining the above working it might be better off being called each_violation. In other words:

on_violation(:property1) do |violation|
  puts "Property 'property1' violated the contract by: #{violation}"
end

on_violation(:property2) do |violation|
  context.fail!(error: "Property2 caused #{violation}")
end

Versus something like

FAILS_ON = [:property2, :property3]
each_violation do |violation, property|
  if FAILS_ON.includes?(property)
    context.fail!(error: "#{property} caused #{violation}")
  else
    puts "Property #{property} violated the contract by: #{violation}"
  else
  end
end

To me, it would be great to support both, because in some scenarios the first option would be briefer/clearer, whereas in other scenarios the second option would be briefer/clearer.

Edit: Also, yes, "method signature" was the term I was looking for but that wouldn't fall out of my brain at that moment :)

[michaelherold]

Hmm, I think your suggestion of two different methods is a great idea. Giving users a way to only handle particular property violations, while also allowing them to easily handle any type of violation. That's a lot of flexibility with a very minimal DSL!

I'm not a big fan of the name #each_violation -- it feels too much like an iterator to me.

What about this?

# Assuming a Violation class similar to this -- could be a symbol violation instead
class Violation
  attr_reader :name
  # Not sure what else we'd want
  # Maybe attr_reader :property
end

# violation = Violation.new('missing')

# I like |property, violation| better, but can see how this is more consistent
on_any_violation do |violation, property|
  context.fail!(message: "#{violation.name}_#{property}"
end

on_violation_of(:property) do |violation|
  context.fail!(message: "#{violation.name}_#{property}")
end

Other options:

• on_violation and on_specific_violation
• handle_any_violation and handle_violation

[jonstokes]

I guess I was thinking that each_violation basically is an iterator, since it would be iterating through some violations list like:

def each_violation
  property_table.each do |property_name, row|
    yield row[:violation], property_name if row[:violation]
  end
end

Anyway, another alternative would just to use on_violation for the above, and then on_violation_for(:property) for a specific property.

[jonstokes]

One other thing I've added in a separate refactor branch that I'll merge soon, is a contract_type method that takes :open or :closed.

In response to @laserlemon's earlier suggestion that it would be a pain to throw errors for context values that aren't declared explicitly, I originally just removed that behavior. But then I thought, this basically makes permits (or allows, whatever we decide to call it) mostly pointless, good only for setting defaults. If we wanted to keep it in as a way to set defaults, it would have to be re-named to something like default_for(:property) { puts "This block is required, or else error" }.

But I personally would at least like the option to enforce the contract more strictly, so I settled on contract_type: :open # or :closed. An open contract is one where you don't have to declare all of the context keys as presence: :permitted or presence: expected. A closed contract throws a violation if it sees a key in context that it doesn't explicitly expect or permit.

All of this leaves the DSL with something like the following:

class MyInteractor
  contract_type :closed # defaults to :open

  expects :property1, :property2, default: :generate_default
  permits(:property3) { :foo }
  provides(:property4) { :bar }

  on_violation_for(:property1) { raise "Property1 is missing!" }

  on_violation do |violation, property|
    next if property == :property1
    if violation.message[/Undeclared property/]
      raise violation.message
    else
      context.fail!(error: "Property '#{property}' violated the contract with #{violation}")
    end
  end
end

[michaelherold]

I guess I was thinking that each_violation basically is an iterator, since it would be iterating through some violations list

Hmm. I didn't get that to start, since everything else about the contract is defining essentially static information on the interactor. I haven't thought about implementation details, just the DSL and what I'd expect to happen, so if that makes more sense from an implementation perspective, I definitely understand going with it. I do like the feel of on_violation and on_violation_for, but I leave it up to you!

One other thing I've added in a separate refactor branch that I'll merge soon, is a contract_type method that takes :open or :closed.

I like the feel of this, particularly with a default to :open. It allows you to be as strict as you need and using a :closed contract would force you to stop any bad habit you might have of adding extra things to a context. How would you imagine a closed contract working in an Organizer? (Just feeling out the UX for it.)

One thought: would it make the code cleaner to separate closed contracts and open contracts from each other by making the closed contract an explicit include for the interactor? I believe with this idea in, we could include the [open] contract module in Interactor by default without breaking backwards compatibility. I'm happy with the DSL approach, just wasn't sure what the code looked like.

expects :property1, :property2, default: :generate_default

This line in the example is a little confusing to me. Would #generate_default be called for both property1 and property2? Or just property2?

[jonstokes]

This line in the example is a little confusing to me. Would #generate_default be called for both property1 and property2? Or just property2?

It would be called for both. With the way that the DSL works right now, if you set a default for any of the regular conditions (expects, permits, provides), then that default applies to every single property in the list of properties for that statement. If you want to set defaults one property at a time, then use separate statements, otherwise the same default goes for all.

[jonstokes]

How would you imagine a closed contract working in an Organizer? (Just feeling out the UX for it.)

I've sort of avoided dealing with surfacing any of this contract stuff explicitly in the organizers, as may have inferred from my lack of reply to your very good "walking the contract chain" suggestion. It's not that I don't like the idea -- I definitely do -- but I almost feel like it's too much to bite off at this point.

I'd rather put together a PR that works really well for providing a contract DSL for the individual interactors, and leave the organizer-level contract implicit for the time being. And then see how people are using it and what the new pain points are that arise.

[michaelherold]

That sounds like a good plan and I agree that it's a lot to bite off at one time. I'm just particularly interested in organizers from a "small, atomic steps" standpoint.

Let's nail down how contracts work in interactors and then once we're to a point where we're happy we can move on to the whole organizer issue.

[jonstokes]

The latest commit has support for the on_violation and on_violation_for stuff described above. One change, though, is that if you want to fail the context then you have to pass the context to the on_violation.. functions, as follows:

  on_violation_for(:property1) { |context| context.fail!(error: "Property1 is missing!") }

  on_violation do |violation, property, context|
    next if property == :property1
    if violation.message[/Undeclared property/]
      raise violation.message
    else
      context.fail!(error: "Property '#{property}' violated the contract with #{violation}")
    end
  end

But otherwise, it all works as described above. See the specs for more details.

Note: I still haven't changed ContextError to ContextViolation and fixed that part of it.

[michaelherold]

This is looking good to me. I'm excited to be able to use contracts in my interactors.

One thought: on_violation do |violation, property, context| feels a little verbose. What about on_violation do |violation, context| and expose the property on the violation? Not sure if that saves much, but it's a thought.

Otherwise my only functionality suggestion is what you already mentioned: ContextViolation and -- my big preference -- PreconditionViolation and PostconditionViolation.

There are a few code suggestions I have that I'll add in-line.

[jonstokes]

@michaelherold I'm hoping to get some time this week to get to the ContextViolation stuff. I like your idea of putting the property in the violation. I too felt like yielding three terms is a bit much. Plus it seems like a natural fit that violation should know what property raised it.

[laserlemon]

Wow, it is really difficult to catch back up on this thread after stepping out for a bit. I really appreciate all the work on this concept of a contract, but I still have significant reservations.

We've been down the road of what we started calling "magical" context access, meaning that you can get and set values on the context via instance methods on the interactor instance. We moved away from that style because it was confusing. I'm a firm believer that although context.foo is longer than foo, it's worth it for clarity's sake. It's especially clear to me when you compare context.foo = "bar" to self.foo = "bar", which not only hides what's going on, but misleads the developer as to where that value is actually set. An interactor is code that reads and manipulates a given context. The illusion of manipulating the interactor instance is confusing for those who are new to the Interactor pattern.

There's also been an emphasis on keeping the Interactor codebase small and easy to understand. These changes more than double Interactor's lines of code, maybe close to triple. I don't think that that fact is bad in itself, but it would certainly add to the maintenance burden and that needs to be considered. My concern is that the heft of these changes might outweigh the value of the features added.

Property defaults are already possible and understandable in before blocks.

before do
  context.tags ||= []
end

I agree that's there's value in the concept of a contract, but this particular implementation has coupled the contract to a couple of features that aren't strictly necessary: the provides method, default values, and reintroducing magical context access.

This comment is a total swoop and :poop: but I don't mean for it to be. I think that the idea of a contract is scratching a real itch that many users are experiencing. I just think that there might be a way to approach it with a lighter touch.

Before we dive back into implementation, we should focus on the API and the developer experience. Here's a really simple Gist where we can discuss and play around with the interface:

https://gist.github.com/laserlemon/71eff42b27e496db2a4f

[jonstokes]

@laserlemon So I take it that removing the magic and making self.foo = :value set an actual instance variable won't be enough to salvage this? Because I'm already part (most?) of the way there with the property_table object that's on the class.

Or what about just ripping out the call to delegate_properties along with that whole method, thereby retaining the context.foo = :value syntax?

[laserlemon]

If what you mean is setting an instance variable on the interactor instance, I'm afraid that would be moving in the wrong direction because we'd be storing internal state on the interactor rather than on the context, as is intended.

I do like the idea of keeping the context.foo = :value syntax, but I'd like for us to start thinking about this in terms of design (What's the most elegant DSL?) rather than implementation (Should we remove delegate_properties?). I know that you and @brianhempel must have had a lot of design discussion prior to submitting the pull request, but I'd like to revisit that conversation because it feels like I'm missing some context (pardon the pun).

[michaelherold]

@laserlemon Thank you for the comments and feedback. I hope I'm not stepping all the maintainers' toes by being so vocal! :smile:

Personally I'm not a fan of doing away with context.foo = :value either for precisely the reasoning that you've mentioned. I was willing to accept self.foo = :value as a means to getting what I really want -- contracts -- since that seemed to be the direction things were heading anyway. What I am interested in is the contract itself; that is, a declarative way to define what the interactor expects and what it outputs. Without that, I find it very frustrating because I have to write a lot of boilerplate code that looks something like this:

class MakeCake
  include Interactor

  # Note: This is the area where I think the DSL should step in
  before :validate_properties
  # after :validate_outputs  -> I don't actually do this, but one could

  def call
    # ...
  end

  private

  def validate_properties
    context.fail!(message: 'missing_flour') unless context.flour.present?
    context.fail!(message: 'missing_egg') unless context.egg.present?
    # ... you get the idea
  end
end

There's not really an expressive way (indeed, this is my new way of doing it which is much clearer than the old method I was using) to do this sort of this in the Interactor DSL. I think this is a hindrance to the gem, primarily for the reason that I've mentioned previous: composing interactors into organizers. Being able to ensure that each step along the way can meet the chain of requirements would be a big win in this department.

Secondly, having the ability to create a "signature" for an interactor is extremely valuable because it makes it easier to grok what's going on in the interactor's behavior. If I can just look at the DSL "preamble," per se, of expects and provides, I know what the interactor does, even if I don't know how it does it. To me, defining the contract would allow a developer to use an interactor without understanding how it works; coupled with "singular" code separation (i.e. single-responsibility code with no side-effects or at least side-effects with rollbacks), you have a beautiful way to onboard a new developer without taking the time to explain the nitty gritty until they need to know it.

I will read through the discussion we've had and distill my idea of what the contract is to add to your gist for discussion purposes. I just hope we can move forward with that concept.

[jonstokes]

If you give direct access to the context, then you have to do away with provides, which I understand @laserlemon does not like. But provides does two things that I think are good, and one thing that I think is necessary:

1) It documents what the interaction adds to the context. 2) It offers a convenient means of setting a default value. 3) It turns the context from a global /object/ that you mutate into a self-documenting /interface/ that you describe and then are forced to use.

I acknowledge that you can do 1 and 2 above easily in a before block with some comments and ||=. But 3 is the the real win.

To me, the whole problem with having the interactors mutate the context willy-nilly is that it's just a global variable, with all of the attendant negatives. You can call it whatever you like, but you're referencing and mutating this global object in multiple source files and accessing it in multiple functions where it's not part any function's method signature, and you're adding and mutating attributes all over the place.

This has a number of bad side effects, not the least of which is that it definitely makes the division of labor among interactors hard to reason about -- the gem gives you no guidance on how to split up the work of transforming the context in a clean and readable manner, and individuals are left to invent idiosyncratic conventions that make the code more readable by the author but not necessarily to an outsider.

By keeping the provides method in the DSL and disallowing direct access to the context object within an interactor, you force people to build and then use an interface around the context, so that they're writing to an interface that they described and documented at the top of the source file (vs. mutating a global variable with attributes that were defined in various other source files). You can't create a new context attribute anywhere in the code by just doing context.new_attr = :bar, because you have to do self.new_attr = :bar and that will throw a NoMethodError, which is appropriate given that you haven't defined new_attr anywhere yet.

The TL;DR is that context.foo = :bar is bad because global variables are bad -- the context is better off as pool of shared state that you CRUD only through a well-defined, self-documenting interface than it is as an object that you mutate wherever and however. And if self.foo = :bar is misleading, then the answer (at least to me) is to make it honest by defining foo as an instance variable and populating it with the corresponding context attribute at the start of the interactor, and then writing its output back to the context object at the end of the interactor. Again, this is "context" as "a repository that your interactor object CRUDs via an interface" vs context as "an object that your interactor mutates".

[jonstokes]

After talking it over with @brianhempel, I've decided I agree with @laserlemon in that there's too much going on here. I like a lot of what I've done because it solves the problem in a way that I understand, and I think that 4.0 should include something along these lines, but I'd rather start using this stuff sooner than later and I think inclusion in 4.0 is too high a bar.

I've made another gem repo that I'm going to move this work to. It will require interactor and just add this contract DSL stuff on top of it. https://github.com/jonstokes/troupe

That way, all this contract-related code, overhead, discussion, etc. is contained, and interactor itself is kept lean and simple. Along the way, if the contract DSL gem gets any traction, maybe we could see what things in it are working in the wild and what things aren't, and that could be useful input into 4.0.

[michaelherold]

I took the basis of our discussion and created a gem out of it that gives you flexible validation of input expectations and output assurances. You can get it here: https://github.com/michaelherold/interactor-contracts

[laserlemon]

Closing in light of the spinoff gems that have resulted.