[WIP] Operation Refactor Sandbox

[jwoertink]

Overrides #363

This will just be a saving sandbox for me to try out some refactors. The actual PRs will be broken out in to smaller ones with less commits allowing them to be easily digested.

The end goal will be:

• Give a unified API between a SaveOperation and a non-db backed Operation
• Allow Operation to use needs, and callbacks
• Cleanup Operation API so the user doesn't need to define a ton of yield blocks
• Moves SaveOperation to a similar API. This API should be very close to how it is now, but maybe with a few minor changes.
• Removed all of the old interfaces that gave errors from deprecated previous versions
• Cleaning up a few compiler errors around operations

Currently in WIP state, so these goals could change, or increase as I get further along in this.

Here's a list of issues that I would like to take a stab at fixing along with this. I can't guarantee all of them, but I want a single place to reference them as I go along.

• - Fix Handle updating nested has_one form #7
• - Fix Issues with blank strings as params #20
• - Fix saving nilable item with form #27
• - Fix Luckyform treats nullable has_one fillable as required fillable #28
• - Fix Raise nicer error if no nested params are in the params sent to a form #41
• - Fix Handle has_many nested forms #43
• - Fix Method for working with VirtualForms more easily #69
• - Fix Add method to params object to check if it has key for a Form #72
• - Fix How to handle when the starting field value is present and an invalid param is sent #83
• - Fix SaveOperation.has_one should still run valid? on nested operation if parent is invalid #276
• - Fix Rescue Lucky::MissingNestedParamError and raise helpful message #281
• - Fix When you try to pass a hash to operation methods like create/update, show how to use named args instead #313
• - Fix Add after_save and before_save callbacks to Factories #331
• - Fix Allow setting different param per attribute #336
• - Fix Param nesting should be optional for Avram::Operation #355
• - Fix has_one should have automatic validator #373
• - Fix SaveOperation fails with blank permitted_columns #377
• - Fix Callbacks do not run if no column attribute changed #432
• - Fix Confusing error when trying to use a nilable attribute #466

[matthewmcgarvey]

Haven't gotten the chance to look through it all yet but I hope this is a useful thought

[matthewmcgarvey]

I see this module is included in Avram::Operation and I'm thinking that these callbacks don't make sense outside of database operations. Could this module be refactored to allow declaring life cycle events that be subscribed to? Then, Avram::Operation could declare before and after run events and Avram::SaveOperation could declare the others. Something like:

module Avram::Callbacks
  macro register_life_cycle_event(event_name, *args)
    macro {{ event_name.id }}(method_name)
      def {{ event_name.id }}(
        {% for param in args %}
          {{ param.id }},
        {% end %}
      )
        \{% if @type.methods.map(&.name).includes?(:before_run.id) %}
          previous_def
        \{% else %}
          super
        \{% end %}

        \{{ method_name.id }}
      end
    end
  end
end

And then in the operations:

abstract class Avram::Operation
  include Avram::Callbacks

  register_life_cycle_event :before_run
  register_life_cycle_event :after_run, object
end

abstract class Avram::SaveOperation(T) < Avram::Operation
  register_life_cycle_event :before_save
  register_life_cycle_event :after_save, record : T
  register_life_cycle_event :after_commit, record : T
end

I have no idea if this actually works but this allows Avram::Callbacks to be a generic utility and not add methods to Avram::Operation that shouldn't be there.

[matthewmcgarvey]

I made a repo with a somewhat working example of my thoughts https://github.com/matthewmcgarvey/crystal-callbacks

[jwoertink]

I like the idea of subscribing to events. Right now there's quite a bit of duplication which I hope to cleanup before this PR is fully done. The one thing I'm unclear on is how we would differentiate between when a method would be called before certain things, or after. Like, this would not stop someone from writing register_life_cycle_event :around_save, but how would that work? I'll have to think on this more after I start to clean up some of the duplication. That might give me a little more clarity. Thanks for the suggestions!

[matthewmcgarvey]

@jwoertink that repo linked above has been updated to work (kind of) as you are wanting. Base classes will call register_event :run and then there will be macro methods for before_run, after_run, and around_run. The macros got quite hairy 🙈

I referenced Rails' callback system for this https://api.rubyonrails.org/classes/ActiveSupport/Callbacks.html

[jwoertink]

Sweet! I'll take a look. Now that I have all these specs passing, I need to start looking at cleaning up some of my duplication and adding in the new bits for this. It's about to get pretty crazy! 😂

[jkthorne]

Will there need to be updates to the guides on the main website?

[jwoertink]

@wontruefree yup luckyframework/website#444 got the issue already. I'm still making some changes to the operations, so even though I'm merging in stuff, I'm trying things out and going back to redo a few things as it gets a little more clear as to how this all should work.

[matthewmcgarvey]

@jwoertink You think we can close out this PR? I have no idea how much of this has been implemented and if there are any issues that can be closed out that are listed.

[jwoertink]

Ok, I don't think I need this anymore. Closing it out!