adomokos / light-service

Series of Actions with an emphasis on simplicity.
MIT License
824 stars 67 forks source link

Deprecation warning using organizer within an organizer #187

Closed alessandro-fazzi closed 4 years ago

alessandro-fazzi commented 4 years ago

Hi,

I'm using an organizer within an organizer; my code has the same structure as the example in the wiki https://github.com/adomokos/light-service/wiki/How-to-create-another-organizer-within-an-organizer, but I'm getting this deprecation warning:

DEPRECATION WARNING: The <[...]> class is an organizer, its entry method (the one that calls with & reduce) should be named `call`. Please use [...].call going forward.

In the main organizer I have

reduce_if(
              lambda do |ctx|
                ctx.database_task &&
                ctx.dig(:global_options, :sql_adapter) == 'wpcli'
              end,
              [Wordmove::Actions::Ssh::WpcliAdapter::PullDb]
            )

and this is the recuced class

module Wordmove
  module Actions
    module Ssh
      module WpcliAdapter
        class PullDb
          extend ::LightService::Action
          extend ::LightService::Organizer
          # [...]

          executed do |context|
            context.logger.task 'Pulling Database'

            # [...]

            with(
              [...]
            ).reduce(
              [
                BackupLocalDb,
                AdaptRemoteDb,
                CleanupAfterPull
              ]
            )
          end
        end
      end
    end
  end
end

What am I doing wrong?

Thanks in advance for your support.

alessandro-fazzi commented 4 years ago

I had a look at https://github.com/adomokos/light-service/blob/2656002f59e56182fb53469afd23b1fa8a36bb0e/lib/light-service/organizer/verify_call_method_exists.rb#L12, but probably it should return also if

klass.included_modules.include? LightService::Action

or (I have not read what klass actually is here, sorry)

klass.singleton_class.included_modules.include? LightService::Action

But as far as I understand the extend order inside implementor's class could become essential for such a check.

Cheers

adomokos commented 4 years ago

When I originally created LS, I had no intention of calling Organizer from an Organizer or an Organizer from an Action, as I felt it might lead to spaghetti code. My goal was to look at an Organizer, see its Actions and based on what I thought where the code I had to modify is, just open that action, edit the code, run the tests, and I am done.

Once you start adding Organizers calling Actions that are calling Organizers in them you are getting into deep nesting which I tried to avoid in the first place.

I don't know what you're trying to do, but we had put together some really complex workflows with the rule of Organizers calling only Actions. Would that work for you as well? Is there a way to refactor your code that way? In case you have a gist where you remove all the sensitive business logic, I am happy to brainstorm with you on how that could be accomplished.

I will remove that page from the Wiki, thank you for pointing out.

gee-forr commented 4 years ago

If you need to call an Organiser from another Organiser, a PR was just merged, see #172, which allows you to expand the actions in the one organiser, into another. It's still nesting like @adomokos mentions above, but a saner way compared to calling the organiser directly.

ni3t commented 4 years ago

Hey all,

I had completely forgotten about #172 until @adomokos merged it yesterday!

In the time since writing that PR, I've actually started using another pattern that works pretty well in our codebase (but, is a little verbose)

class SomeOrganizer
  extend LightService::Organizer

  def self.call(foo:, bar:)
    with(foo: foo, bar: bar).reduce(actions)
  end

  def self.actions
    [...]
  end

  class AsAction
    extend LightService::Action

    expects :foo, :bar 

    executed do |c|
      result = module_parent.call(foo: foo, bar: bar)
      c.fail_and_return!(result.message, error_code: result.error_code) if result.failure?
    end
  end
end

class OtherOrganizer
  extend LightService::Organizer

  def self.call(bar:, foo:)
    with(bar: bar, foo: foo).reduce(actions)
  end

  def self.actions
    [ SomeOrganizer::AsAction, ... ]
  end
end

This allows us to nest organizers in a predictable and clean fashion without having to rely on LightService's internals or using an Organizer as an Action directly. We ran into some pretty nasty bugs using Organizer.actions, since it took a significant amount of code tracing to figure out where certain actions were being called. It also allowed us to rename context attributes on the fly, as well as avoid naming collisions if multiple organizers had the same keys.

Using module_parent is just a ruby shortcut since our organizers are typically name-spaced 4 levels deep and it can be a pain to write the long version.

Hope this helps!

adomokos commented 4 years ago

That's quite a bit of code gymnastics you have to do. If it works, 👍.

I also added ContextFactory to aid the testing of actions, but you need to eliminate mocking and talk to a database in your tests.

Thanks for your contribution, @ni3t! #172 is a great addition to LS!

adomokos commented 4 years ago

@pioneerskies, I closed the issue, but let me know if you disagree or you think we should have an action item from the conversation here.

alessandro-fazzi commented 4 years ago

Hello @adomokos,

thanks for your help and thanks to other people to have posted on this issue.

No problem closing this one: since I'm experimenting using LS to refactor a project from upside down, I've absolutely no problem adopting a different pattern than nested organizers. Sincerly I've tried it out because it was documented 😄

My main motivations were 2:

  1. creating a subcontext instead of having a fat single one, because a specific group in my actions does need specific data
  2. having less actions and less setup logic in the main organizer

Both motivations are a bit naive and just a spot along the path to find a balanced and readable design and code organization. Probably I'll simply opt for transforming the organizer into an action dedicated to setup data in the context for the subsequent ones :)

Should I have any block concerning the use of LS I'll ask for help in another issue.

Thanks so much for the appreciated support