New Interface for Operations #469

jwoertink · 2020-09-23T20:07:59Z

Fixes #69

This PR is part of a series which breaks out #369 in to much smaller (and easier to digest) PRs. I'll try to best explain everything going on here.

In this PR SaveOperation no longer inherits from Operation, and Operation now has an interface which makes it feel a lot closer to how SaveOperation works. This means we can now do this:

class SignInUser < Avram::Operation
  needs http_context : HTTP::Context
  attribute email : String
  attribute password : String
  attribute remember_me : Bool
  file_attribute :biometric_retinal_scan

  before_run do
    check_login_count(http_context)
    validate_required email, password
  end

  after_run do |user|
    save_successful_login(user)
  end

  def run
    # do some fancy login logic
    new_current_user
  end
end

post "/login" do
  SignInUser.run(params, http_context: context) do |operation, user|
    if user
      store_user_and_redirect(user)
    else
       you_get_the_idea
    end
  end
end

All of the SaveOperation work exactly the same as before.

…commits related to #369

…use both attribute and file_attribute

jwoertink · 2020-09-23T20:13:33Z

src/avram/callbacks.cr

@@ -24,6 +24,18 @@ module Avram::Callbacks
    end
  end

+  macro before_run(method_name)


This does the exact same thing as before_save, but it feels weird calling before_save in an Operation. To clean this up, we could just have before_run call before_save internally... but then if we ever needed before_save to act different, we'd end up having to break them out again.

This is some tricky naming. What about before_execution?

The method that you call in these is called run. So like LoginUser.run(params). This was actually sort of modeled after the mutations gem in Ruby 😅 (though, that doesn't have callbacks).

I think I'd flip it. So before_save calls before_run under the hood. That way if before_save needs to change something we can modify just before_save and leave before_run alone

jwoertink · 2020-09-23T20:13:46Z

src/avram/callbacks.cr

@@ -47,6 +59,18 @@ module Avram::Callbacks
    end
  end

+  macro before_run


jwoertink · 2020-09-23T20:14:24Z

src/avram/callbacks.cr

+  #   end
+  # end
+  # ```
+  macro after_run(&block)


The after_save equivalent doesn't exist yet, but will in a following PR to keep this as small as I can.

jwoertink · 2020-09-23T20:16:01Z

src/avram/needy_initializer.cr

@@ -0,0 +1,102 @@
+module Avram::NeedyInitializer


This module covers everything the needy_initializer_with_save_methods does, but without the save methods since those are related to SaveOperation specifically. I made a separate one because I had a lot of trouble with the compiler trying to tell "if it's SaveOp, then call these macros, else skip them". It was easier to just break them apart.

jwoertink · 2020-09-23T20:17:34Z

src/avram/operation.cr

-end
-
-class Avram::Operation
+abstract class Avram::Operation


If we wanted an extra level of type-safety, we could make this a Generic, and ensure run returns T. 🤔 Some people may just want it to run a task and be void at the end though..

jwoertink · 2020-09-23T20:18:43Z

src/avram/operation.cr

-
-class Avram::Operation
+abstract class Avram::Operation
+  include Avram::NeedyInitializer
  include Avram::DefineAttribute
  include Avram::Validations
  include Avram::SaveOperationErrors


This isn't really SaveOperation specific. I could rename it, or that could come in a future PR I'm planning with some cleanup stuff.

Future PR sounds good! Avram::OperationsErrors seems like a good name

matthewmcgarvey

I was nervous that you were going to try to merge the monster PR 🤣

src/avram/callbacks.cr

src/avram/needy_initializer.cr

matthewmcgarvey · 2020-09-23T20:41:20Z

src/avram/needy_initializer.cr

+  macro inherit_needs
+    \{% if !@type.constant(:OPERATION_NEEDS) %}
+      OPERATION_NEEDS = [] of Nil
+    \{% end %}


This doesn't seem necessary...

🤷 lol. I know it's bad, but I literally copied and just ripped out code I assumed wasn't needed. I guess I should actually read what it is doing 😂

I took a look at this again, and without it, things start to explode, so I'm leaving it in for now.

Co-authored-by: Matthew McGarvey <matthewmcgarvey14@gmail.com>

…s called

jwoertink · 2020-10-14T20:00:51Z

@confact and @stephendolan since both of you have Lucky apps out in the wild, would you mind taking a glance over this to see if something is missing? When you get a free moment, of course.

I'm mainly looking for examples of regular Avram::Operation you might have in your apps where the code is a bit messy and see if this would clean that up and allow you to do what you need still. Or if you see something that makes you say "if this got merged, my app would break with no way to fix it"... Thanks! 😄

src/avram/callbacks.cr

src/avram/operation.cr

… bang method return the value instead of the operation to be consistent with SaveOperation bang methods

…vram into separate_save_operation

jwoertink · 2020-10-15T16:08:16Z

Thanks for the review @matthewmcgarvey! Those changes definitely help to make this code easier to read. If you have any ideas for more specs I can add in, let me know!

matthewmcgarvey

Looks good 👍

jwoertink · 2020-10-15T19:15:53Z

Sweet! I'm gonna merge and start prepping the next PR for this series.

paulcsmith

This is really awesome!! Thank you

paulcsmith · 2020-10-21T20:39:07Z

src/avram/callbacks.cr

@@ -24,6 +24,18 @@ module Avram::Callbacks
    end
  end

+  macro before_run(method_name)


I think I'd flip it. So before_save calls before_run under the hood. That way if before_save needs to change something we can modify just before_save and leave before_run alone

paulcsmith · 2020-10-21T20:40:56Z

src/avram/operation.cr

-
-class Avram::Operation
+abstract class Avram::Operation
+  include Avram::NeedyInitializer
  include Avram::DefineAttribute
  include Avram::Validations
  include Avram::SaveOperationErrors


Future PR sounds good! Avram::OperationsErrors seems like a good name

jwoertink added 2 commits September 23, 2020 11:36

Separating SaveOperation from Operation. This is part of a series of …

817c336

…commits related to #369

Added missing specs for Operation attributes to ensure Operation can …

391413e

…use both attribute and file_attribute

jwoertink added clarify api Rename/remove/add something to make the API easier to understand BREAKING CHANGE This will cause a breaking change labels Sep 23, 2020

jwoertink requested a review from paulcsmith September 23, 2020 20:08

jwoertink commented Sep 23, 2020

View reviewed changes

src/avram/callbacks.cr

@@ -47,6 +59,18 @@ module Avram::Callbacks

end

end

macro before_run

Copy link

Member Author

jwoertink Sep 23, 2020

Choose a reason for hiding this comment

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

ditto

jwoertink commented Sep 23, 2020

View reviewed changes

matthewmcgarvey reviewed Sep 23, 2020

View reviewed changes

jwoertink and others added 3 commits September 23, 2020 13:55

Update src/avram/needy_initializer.cr

d842a18

Co-authored-by: Matthew McGarvey <matthewmcgarvey14@gmail.com>

added some docs and updated Operation to ensure the FailedOperation i…

cb9adb0

…s called

Merge branch 'master' into separate_save_operation

2ef2abf

matthewmcgarvey requested changes Oct 14, 2020

View reviewed changes

src/avram/callbacks.cr Outdated Show resolved Hide resolved

src/avram/callbacks.cr Outdated Show resolved Hide resolved

src/avram/callbacks.cr Outdated Show resolved Hide resolved

src/avram/operation.cr Outdated Show resolved Hide resolved

jwoertink added 5 commits October 15, 2020 08:31

Merge branch 'master' into separate_save_operation

26633d4

Refactor callbacks to remove some of the duplication.

d614beb

Refactor run method to clean up some duplication. Also having the run…

a9ae063

… bang method return the value instead of the operation to be consistent with SaveOperation bang methods

Merge branch 'separate_save_operation' of github.com:luckyframework/a…

fdd9cc2

…vram into separate_save_operation

ran ameba

70772f5

matthewmcgarvey approved these changes Oct 15, 2020

View reviewed changes

jwoertink merged commit 69e61d6 into master Oct 15, 2020

jwoertink deleted the separate_save_operation branch October 15, 2020 19:16

This was referenced Oct 15, 2020

before_save callbacks may run multiple times #480

Closed

Document new Operations luckyframework/website#444

Closed

paulcsmith reviewed Oct 21, 2020

View reviewed changes

ZimbiX mentioned this pull request Nov 1, 2020

lucky dev: When Ctrl+C is pressed, wait for the child process to handle the signal and finish - rather than us immediately exiting and leaving it to log shutting down messages from the background luckyframework/lucky_cli#572

Merged

jwoertink mentioned this pull request Nov 2, 2020

Update generated Operations to new structure luckyframework/lucky_cli#574

Closed

jwoertink mentioned this pull request Nov 13, 2020

Some simple cleanup on the path to #369 #532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Interface for Operations #469

New Interface for Operations #469

jwoertink commented Sep 23, 2020

jwoertink Sep 23, 2020

jkthorne Oct 15, 2020

jwoertink Oct 15, 2020

paulcsmith Oct 21, 2020

jwoertink Sep 23, 2020

jwoertink Sep 23, 2020

jwoertink Sep 23, 2020

jwoertink Sep 23, 2020

jwoertink Sep 23, 2020

paulcsmith Oct 21, 2020

matthewmcgarvey left a comment

matthewmcgarvey Sep 23, 2020

jwoertink Sep 23, 2020

jwoertink Oct 15, 2020

jwoertink commented Oct 14, 2020

jwoertink commented Oct 15, 2020

matthewmcgarvey left a comment

jwoertink commented Oct 15, 2020

paulcsmith left a comment

paulcsmith Oct 21, 2020

paulcsmith Oct 21, 2020

New Interface for Operations #469

New Interface for Operations #469

Conversation

jwoertink commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewmcgarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoertink commented Oct 14, 2020

jwoertink commented Oct 15, 2020

matthewmcgarvey left a comment

Choose a reason for hiding this comment

jwoertink commented Oct 15, 2020

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment