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

[WIP] Invoke commands to combine #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IvanShamatov
Copy link
Member

I'm working on idea of invoking commands from other commands to combine them together.
Say you have views, model, controller generators and you want to make a scaffold command

module Generate
  class Scaffold < Dry::CLI::Command
    argument :model_name

    def call(model_name:)
      invoke(%w[g model], model_name)
      invoke(%w[g controller], model_name)
    end
  end

  class Model < Dry::CLI::Command
    argument :model_name

    def call(model_name:)
      invoke(%[g migration], model_name)
      some_other_model_staff
    end
  end

  class Controller < Dry::CLI::Command
    argument :model_name

    def call(model_name:)
      add_routes
      add_controller
    end

    def add_controller(model_name)
      ACTIONS.each { |action|  invoke(%[g views], model_name, action) }
      some_other_controller_staff
    end
  end
end

WDYT ?

@IvanShamatov IvanShamatov self-assigned this Jan 29, 2020
@solnic
Copy link
Member

solnic commented Jan 29, 2020

This is a useful feature. I already needed it in a couple of places.

@jodosha
Copy link
Member

jodosha commented Jan 29, 2020

@IvanShamatov This is useful, indeed. What I suggest if it's possible to use message passing, instead of system calls to execute other commands it would be preferrable. But if that is the cost of messing with the command name registry then system calls are fine.

Please invest a little bit more time in thinking what happens if the invoked command crashes.

@solnic
Copy link
Member

solnic commented Jan 30, 2020

I think this should just use command objects - is it not possible/feasible now? I do that easily in tests so...

@IvanShamatov
Copy link
Member Author

@solnic I do not pass registry into commands, but to be able to invoke commands from commands expect registry be injected into command object. Actually I'm thinking about that way of implementation and will definitely try it in the nearest time.

@IvanShamatov IvanShamatov force-pushed the feature/invoke-commands-from-other-commands branch 2 times, most recently from 636dfb9 to 40d202a Compare January 30, 2020 22:41
@IvanShamatov IvanShamatov force-pushed the feature/invoke-commands-from-other-commands branch from 40d202a to 55fd722 Compare January 30, 2020 22:49
@dry-bot
Copy link
Collaborator

dry-bot commented Jan 30, 2020

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 2
           

See the complete overview on Codacy

@@ -27,6 +27,7 @@ Gem::Specification.new do |spec|

# to update dependencies edit project.yml
spec.add_runtime_dependency "concurrent-ruby", "~> 1.0"
spec.add_runtime_dependency "dry-effects"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@IvanShamatov as the comment states: to update dependencies you need to edit project.yml 😄

@@ -1,4 +1,5 @@
# frozen_string_literal: true
require 'dry/effects'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@adrianthedev
Copy link
Contributor

+1 from me too.

Thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants