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

AASM generator #463

Merged
merged 3 commits into from
Sep 3, 2021
Merged

AASM generator #463

merged 3 commits into from
Sep 3, 2021

Conversation

kddnewton
Copy link
Contributor

@kddnewton kddnewton commented Aug 27, 2021

This commit adds a generator for the AASM state machine gem. AASM allows you to define a state machine with states, events, and transitions. Each of these generates various things in the owning class.

  • For each state xxx
    • A constant STATE_XXX with the value that is stored in the state column
    • A method xxx? that allows you to query for if the owning class is in that state
  • For each event xxx
    • A method xxx that triggers that event
    • A method xxx! that triggers that event and potentially raises an error
    • A method xxx_without_validation! that triggers that event and skips validation
    • A method may_xxx? that runs the validation but does not trigger that event

This gem relies very heavily on instance_eval. For example, in the follow snippet:

class StateMachine
  include AASM
  extend T::Sig

  aasm do
    # This block is evaluated in the context of a new AASM::Base object.
    # We _could_ shim this class to get the right binding for the aasm
    # method, but see below.

    state :sleeping, initial: true
    state :running, :cleaning

    event :run do
      # This block is evaluated in the context of a new
      # AASM::Core::Event object. We could also shim this class to get
      # the right binding for the event method, but see below.

      before do
        # This block is evaluated all of the way back up in the
        # StateMachine class, which means that before_run here will
        # actually evaluate properly. So even if we were to shim the
        # aasm and event methods, there's no way to shim it such that
        # this method call will evaluate without either
        # T.bind(T.attached_class) which doesn't exist or forcing the
        # consumers to always T.bind(self, StateMachine) within each
        # callback.
        before_run
      end

      transitions from: :sleeping, to: :running
    end
  end

  private

  sig { void }
  def before_run; end
end

To get around the problems mentioned in the snippet above, we generate a private PrivateAASMMachine constant within each class that includes a state machine where we properly bind the callback methods. You can see the spec for a detailed layout of what gets generated.

@RyanBrushett I ended up keeping around the private classes just because they made it more of a complete implementation here. Appears to work.

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I just have a small test addition suggestion.

it("generates empty RBI file if this is no state machine") do
add_ruby_file("content.rb", <<~RUBY)
class StateMachine
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens for the case when the class includes AASM but has not made a call to aasm yet? Can we test for that too please?

Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this IRL with a couple of projects and it worked great. Thanks!

This commit adds a generator for the AASM state machine gem. AASM allows you to define a state machine with states, events, and transitions. Each of these generates various things in the owning class.

* For each state `xxx`
  * A constant `STATE_XXX` with the value that is stored in the state column
  * A method `xxx?` that allows you to query for if the owning class is in that state
* For each event `xxx`
  * A method `xxx` that triggers that event
  * A method `xxx!` that triggers that event and potentially raises an error
  * A method `xxx_without_validation!` that triggers that event and skips validation
  * A method `may_xxx?` that runs the validation but does not trigger that event

This gem relies very heavily on `instance_eval`. For example, in the follow snippet:

```ruby
class StateMachine
  include AASM
  extend T::Sig

  aasm do
    # This block is evaluated in the context of a new AASM::Base object.
    # We _could_ shim this class to get the right binding for the aasm
    # method, but see below.

    state :sleeping, initial: true
    state :running, :cleaning

    event :run do
      # This block is evaluated in the context of a new
      # AASM::Core::Event object. We could also shim this class to get
      # the right binding for the event method, but see below.

      before do
        # This block is evaluated all of the way back up in the
        # StateMachine class, which means that before_run here will
        # actually evaluate properly. So even if we were to shim the
        # aasm and event methods, there's no way to shim it such that
        # this method call will evaluate without either
        # T.bind(T.attached_class) which doesn't exist or forcing the
        # consumers to always T.bind(self, StateMachine) within each
        # callback.
        before_run
      end

      transitions from: :sleeping, to: :running
    end
  end

  private

  sig { void }
  def before_run; end
end
```

To get around the problems mentioned in the snippet above, we generate a private PrivateAASMMachine constant within each class that includes a state machine where we properly bind the callback methods. You can see the spec for a detailed layout of what gets generated.
assert_equal(expected, rbi_for(:StateMachine))
end

it "generates RBI when AASM is included but no AASM call has been made" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paracycle Is this what you had in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for what is being tested. However, I would have expected the output to be empty if there are no states on the state machine. Maybe we should make it so.

Copy link
Contributor

@RyanBrushett RyanBrushett Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

@RyanBrushett RyanBrushett merged commit 04ad3f8 into main Sep 3, 2021
@RyanBrushett RyanBrushett deleted the aasm branch September 3, 2021 16:56
@kddnewton
Copy link
Contributor Author

Thanks for the follow-up @RyanBrushett it was so nice to come back from vacation and see this merged :)

@shopify-shipit shopify-shipit bot temporarily deployed to production September 7, 2021 16:35 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants