Add Rails commands for existing rake tasks #21254

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@ccallebs
Contributor

ccallebs commented Aug 16, 2015

Aims to implement #18878.

This is a proof of concept branch. The code is a bit messy and I've left comments where I plan to clean it up. I'm also not sure what dependencies will end up being shared among tasks, so a lot of functionality is in the Base class at the moment.

Placing each set of rake tasks inside of a wrapper class allows some flexibility for the future in terms of providing contextual documentation. For example, rails test --help could provide a custom help message for only test commands (and encapsulate those messages inside of each class).

Currently working:

  • Original commands
  • rails test:db

Before I go any further moving more rake tasks to this architecture, I wanted to get feedback on the implementation. I think this approach is sensible and I can knock out the rest of them quickly.

@repinel

View changes

railties/lib/rails/commands/commands_tasks.rb
module Rails
# This is a class which takes in a rails command and initiates the appropriate
# initiation sequence.
#
# Warning: This class mutates ARGV because some commands require manipulating
# it before they are run.
- class CommandsTasks # :nodoc:
+ class CommandsTasks # :nodoc:

This comment has been minimized.

@repinel

repinel Aug 17, 2015

Member

✂️

@repinel

repinel Aug 17, 2015

Member

✂️

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 19, 2015

Contributor

Just pinging here -- is this a direction I should continue going down?

Contributor

ccallebs commented Aug 19, 2015

Just pinging here -- is this a direction I should continue going down?

@kaspth

View changes

railties/lib/rails/commands/task_builder/test.rb
+ # This is a wrapper around all Rails test tasks, including:
+ # rails test
+ # rails test:db
+ class Test

This comment has been minimized.

@kaspth

kaspth Aug 19, 2015

Member

Why add a base class and then not inherit from it?

@kaspth

kaspth Aug 19, 2015

Member

Why add a base class and then not inherit from it?

This comment has been minimized.

@ccallebs

ccallebs Aug 19, 2015

Contributor

@kaspth Base was probably a bad name. It's really a wrapper for all existing rails commands. Perhaps I'll rename "Base" to "Common" and "Common" to "HelperMethods".

@ccallebs

ccallebs Aug 19, 2015

Contributor

@kaspth Base was probably a bad name. It's really a wrapper for all existing rails commands. Perhaps I'll rename "Base" to "Common" and "Common" to "HelperMethods".

This comment has been minimized.

@matthewd

matthewd Aug 19, 2015

Member

Without implying any comment on the overall structure, which I don't think I've fully understood yet... I'd suggest "Core" for that class name.

@matthewd

matthewd Aug 19, 2015

Member

Without implying any comment on the overall structure, which I don't think I've fully understood yet... I'd suggest "Core" for that class name.

This comment has been minimized.

@ccallebs

ccallebs Aug 19, 2015

Contributor

Good suggestion. Let me know if you have any questions about the overall structure.

@ccallebs

ccallebs Aug 19, 2015

Contributor

Good suggestion. Let me know if you have any questions about the overall structure.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 19, 2015

Member

Much of this feels like adding more wiring for the sake of wiring. If we are to refactor this, what a task does would make most sense alongside how to run that task.

Ideally this gets us our own lightweight task system. One that lets us write better CLIs rather than deal with cumbersome ENV fidgery. The pure plumbing of running the tasks should still be Rake.

Am I right about this being where we want to go, @dhh @senny @matthewd @rafaelfranca?

Member

kaspth commented Aug 19, 2015

Much of this feels like adding more wiring for the sake of wiring. If we are to refactor this, what a task does would make most sense alongside how to run that task.

Ideally this gets us our own lightweight task system. One that lets us write better CLIs rather than deal with cumbersome ENV fidgery. The pure plumbing of running the tasks should still be Rake.

Am I right about this being where we want to go, @dhh @senny @matthewd @rafaelfranca?

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Aug 19, 2015

Member

I don't think I'm quite following why we're rearranging all the command-handling for the existing rails subcommands here... don't they already do what we want?

Member

matthewd commented Aug 19, 2015

I don't think I'm quite following why we're rearranging all the command-handling for the existing rails subcommands here... don't they already do what we want?

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 19, 2015

Contributor

@matthewd @kaspth With the full list of commands we'll have to support, we'll need some sort of organization structure vs. including everything in the CommandsTasks class. See: #18878 (comment)

However: this is a moot point if whitelisting commands isn't wanted. The added wiring wouldn't have any benefit in that case. I'll add some more examples so my intent is more clear.

Contributor

ccallebs commented Aug 19, 2015

@matthewd @kaspth With the full list of commands we'll have to support, we'll need some sort of organization structure vs. including everything in the CommandsTasks class. See: #18878 (comment)

However: this is a moot point if whitelisting commands isn't wanted. The added wiring wouldn't have any benefit in that case. I'll add some more examples so my intent is more clear.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 19, 2015

Contributor

@matthewd @kaspth Committed an example of what the "rails tmp" and "rails assets" commands would look like in this structure. I'll put some work in this evening to clean up the code.

Contributor

ccallebs commented Aug 19, 2015

@matthewd @kaspth Committed an example of what the "rails tmp" and "rails assets" commands would look like in this structure. I'll put some work in this evening to clean up the code.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 20, 2015

Contributor

I cleaned up the code a bit and added a few more examples.

Contributor

ccallebs commented Aug 20, 2015

I cleaned up the code a bit and added a few more examples.

@kaspth

View changes

railties/lib/rails/commands/warden.rb
+ def run!
+ method_name = command.gsub(/:/, "_")
+
+ if Rails::Commands::Core::COMMAND_WHITELIST.include?(command)

This comment has been minimized.

@kaspth

kaspth Aug 20, 2015

Member

This whole if-statement is just silly. We need something more flexible than this.

@kaspth

kaspth Aug 20, 2015

Member

This whole if-statement is just silly. We need something more flexible than this.

This comment has been minimized.

@ccallebs

ccallebs Aug 20, 2015

Contributor

@kaspth I agree. I reworked it, but it still seems hacky to me. Let me know if you have any suggestions.

@ccallebs

ccallebs Aug 20, 2015

Contributor

@kaspth I agree. I reworked it, but it still seems hacky to me. Let me know if you have any suggestions.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 20, 2015

Member

This shows more promise.

I'm sure we could clean up some of the code be having a Command base class. I think it could also be great if we follow the pattern in Action Controller and other libraries where any public method is an action, i.e. a callable whitelisted command.

Here's a rough skeleton of what I'm thinking:

class Command
  attr_reader :argv

  def intiailize(argv)
    @argv = argv
  end

  def run(task_name)
    command_name = command_name_for(task_name)

    if command = command_for(command_name)
      command.public_send(command_name)
    else
      # Print help or some other documentation
    end
  end

  def tasks
    public_instance_methods.map { |method| method.gsub('_', ':') }
  end

  private
    @commands = []

    def self.inherited(command)
      @commands << command
    end

    def command_for(task_name)
      @commands.find { |command| command.respond_to?(command_name) }
    end

    def command_name_for(task_name)
      task_name.gsub(':', '_')
    end
end

It would be great if we could make commands know how to run themselves and then perhaps having a class method do the routing of finding the right command. Exactly like Minitest does with runnables.

Member

kaspth commented Aug 20, 2015

This shows more promise.

I'm sure we could clean up some of the code be having a Command base class. I think it could also be great if we follow the pattern in Action Controller and other libraries where any public method is an action, i.e. a callable whitelisted command.

Here's a rough skeleton of what I'm thinking:

class Command
  attr_reader :argv

  def intiailize(argv)
    @argv = argv
  end

  def run(task_name)
    command_name = command_name_for(task_name)

    if command = command_for(command_name)
      command.public_send(command_name)
    else
      # Print help or some other documentation
    end
  end

  def tasks
    public_instance_methods.map { |method| method.gsub('_', ':') }
  end

  private
    @commands = []

    def self.inherited(command)
      @commands << command
    end

    def command_for(task_name)
      @commands.find { |command| command.respond_to?(command_name) }
    end

    def command_name_for(task_name)
      task_name.gsub(':', '_')
    end
end

It would be great if we could make commands know how to run themselves and then perhaps having a class method do the routing of finding the right command. Exactly like Minitest does with runnables.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 20, 2015

Contributor

@kaspth Everything you said is clear except the last part. What do you mean when you say making commands that know how to run themselves? Could you give a usage example?

Contributor

ccallebs commented Aug 20, 2015

@kaspth Everything you said is clear except the last part. What do you mean when you say making commands that know how to run themselves? Could you give a usage example?

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 21, 2015

Contributor

@kaspth I implemented your suggestions. It made it a lot cleaner for sure. If you're cool with the code, I can go ahead and implement the other commands and start writing tests around each of them.

Contributor

ccallebs commented Aug 21, 2015

@kaspth I implemented your suggestions. It made it a lot cleaner for sure. If you're cool with the code, I can go ahead and implement the other commands and start writing tests around each of them.

@kaspth

View changes

railties/lib/rails/commands/command.rb
+ @@command_wrappers << command
+ end
+
+ def command_instance_for(task_name)

This comment has been minimized.

@kaspth

kaspth Aug 23, 2015

Member

This should probably take a command_name as that's what's passed in run.

@kaspth

kaspth Aug 23, 2015

Member

This should probably take a command_name as that's what's passed in run.

@kaspth

View changes

railties/lib/rails/commands/command.rb
+ klass = @@command_wrappers.find do |command|
+ command.method_defined?(command_name)
+ end
+ klass.new(@argv)

This comment has been minimized.

@kaspth

kaspth Aug 23, 2015

Member

Add a new line above this

@kaspth

kaspth Aug 23, 2015

Member

Add a new line above this

@kaspth

View changes

railties/lib/rails/commands/command.rb
+ def command_instance_for(task_name)
+ command_name = command_name_for(task_name)
+ klass = @@command_wrappers.find do |command|
+ command.method_defined?(command_name)

This comment has been minimized.

@kaspth

kaspth Aug 23, 2015

Member

We have to make sure this only returns true for public methods. Protected and private shouldn't be marked as runnable tasks.

@kaspth

kaspth Aug 23, 2015

Member

We have to make sure this only returns true for public methods. Protected and private shouldn't be marked as runnable tasks.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 23, 2015

Member

Yeah, sorry that was hazy. My point was just that I didn't want a separate router/manager. The commands should know how to run themselves. Akin to Runnables in Minitest: https://github.com/seattlerb/minitest/blob/master/lib/minitest.rb#L104-L111

I think this direction is much better overall. It gives a solid footing with backwards compatibility while letting us task by task enrich them 👍

But I'd like to get some feedback from others before we get too far into this. cc @matthewd @senny @rafaelfranca @arthurnn

Member

kaspth commented Aug 23, 2015

Yeah, sorry that was hazy. My point was just that I didn't want a separate router/manager. The commands should know how to run themselves. Akin to Runnables in Minitest: https://github.com/seattlerb/minitest/blob/master/lib/minitest.rb#L104-L111

I think this direction is much better overall. It gives a solid footing with backwards compatibility while letting us task by task enrich them 👍

But I'd like to get some feedback from others before we get too far into this. cc @matthewd @senny @rafaelfranca @arthurnn

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 23, 2015

Member

As a side note most of the commands will start out as mere wrappers around a rake task at first. We could make those easier to write with:

class Assets < Command
  rake_delegate 'assets:precompile'
end

class Command
  def self.rake_delegate(task_name)
    define_method(command_name_for(task_name)) do
      Rake::Task[task_name].invoke
    end
  end
end
Member

kaspth commented Aug 23, 2015

As a side note most of the commands will start out as mere wrappers around a rake task at first. We could make those easier to write with:

class Assets < Command
  rake_delegate 'assets:precompile'
end

class Command
  def self.rake_delegate(task_name)
    define_method(command_name_for(task_name)) do
      Rake::Task[task_name].invoke
    end
  end
end
@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 24, 2015

Contributor

@kaspth That's good suggestion, I like that. I'll spend some time this week implementing that, finishing the rest of the supported commands, and getting tests around everything. One thing we've not talked about is documentation changes. Do you have a clear vision for what that should look like?

Contributor

ccallebs commented Aug 24, 2015

@kaspth That's good suggestion, I like that. I'll spend some time this week implementing that, finishing the rest of the supported commands, and getting tests around everything. One thing we've not talked about is documentation changes. Do you have a clear vision for what that should look like?

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 25, 2015

Member

Well, what we do with documentation depends on how much we want to spit out on the command line.

On the one hand people would probably expect a lot of output through rails test --help or similar. But we also need make sure the documentation is visible online through with what RDoc generates.

We also don't want to write the documentation twice. So either we add an API that lets us annotate commands with strings of documentation (some wrapper around OptionParser most likely) thats funneled through RDoc somehow. Or we add standard comments that we spit into OptionParser too somehow.

@zzak do you have any input on how we best add documentation here?

Member

kaspth commented Aug 25, 2015

Well, what we do with documentation depends on how much we want to spit out on the command line.

On the one hand people would probably expect a lot of output through rails test --help or similar. But we also need make sure the documentation is visible online through with what RDoc generates.

We also don't want to write the documentation twice. So either we add an API that lets us annotate commands with strings of documentation (some wrapper around OptionParser most likely) thats funneled through RDoc somehow. Or we add standard comments that we spit into OptionParser too somehow.

@zzak do you have any input on how we best add documentation here?

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 28, 2015

Contributor

Just popping back in to say this hasn't fallen off my radar. I'll be traveling this weekend and plan to get a lot of this done on the ✈️.

Contributor

ccallebs commented Aug 28, 2015

Just popping back in to say this hasn't fallen off my radar. I'll be traveling this weekend and plan to get a lot of this done on the ✈️.

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Aug 30, 2015

Member

@kaspth I haven't followed the entire discussion so I'm not sure, tbh.

Member

zzak commented Aug 30, 2015

@kaspth I haven't followed the entire discussion so I'm not sure, tbh.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 16, 2015

Member

@ccallebs how long was the flight? 😁

Member

kaspth commented Sep 16, 2015

@ccallebs how long was the flight? 😁

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Sep 16, 2015

Contributor

@kaspth I got work done during it, just didn't finish the whole thing :). I'll need to wrap the remaining commands + add the tests. Also modify the documentation (not sure what that will look like). What kind of feedback are we waiting on from the other interested parties?

Contributor

ccallebs commented Sep 16, 2015

@kaspth I got work done during it, just didn't finish the whole thing :). I'll need to wrap the remaining commands + add the tests. Also modify the documentation (not sure what that will look like). What kind of feedback are we waiting on from the other interested parties?

@kaspth kaspth self-assigned this Sep 16, 2015

@kaspth kaspth added the railties label Sep 16, 2015

@kaspth kaspth added this to the 5.0.0 milestone Sep 16, 2015

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 16, 2015

Member

Nice! 😄

I'll try to come up with something for the documentation tomorrow.

Just that they agree with the direction here 😉 (cc @rafaelfranca @senny @arthurnn @matthewd)

Member

kaspth commented Sep 16, 2015

Nice! 😄

I'll try to come up with something for the documentation tomorrow.

Just that they agree with the direction here 😉 (cc @rafaelfranca @senny @arthurnn @matthewd)

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Sep 16, 2015

Contributor

@kaspth Cool, I'll be watching this PR closely for the 🚥.

Contributor

ccallebs commented Sep 16, 2015

@kaspth Cool, I'll be watching this PR closely for the 🚥.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Sep 16, 2015

Contributor

@kaspth In terms of the documentation, It seems to me that we might want to store the contextual messages in the separate command classes themselves. Not sure if that's what you were thinking, but it seems like a logical place for it.

Contributor

ccallebs commented Sep 16, 2015

@kaspth In terms of the documentation, It seems to me that we might want to store the contextual messages in the separate command classes themselves. Not sure if that's what you were thinking, but it seems like a logical place for it.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 17, 2015

Member

Oh yeah, we'd definitely want the documentation on the commands themselves.

So for the tasks that are already run with rails we don't have to do anything about the documentation. And we can just follow their example when porting over the rake tasks.

The rails commands just rely on OptionParser within themselves and then there's the Rails command line guide for anything more advanced documentation.

So the commands should only be able to extend the option parser:

module Rails
  class Command
    def initialize(argv)
      @argv = argv

      @option_parser = build_option_parser
      @options = {}
    end

    def run(task_name)
      command_name = command_name_for(task_name)

      parse_options_for(command_name)        
      @option_parser.parse! @argv

      if command = command_for(command_name)
        command.public_send(command_name)
      else
        puts @option_parser
      end
    end

    def self.options_for(command_name, &options_to_parse)
      @@command_options[command_name] = options_to_parse
    end

    private
      @@command_options = {}

      def parse_options_for(command_name)
        @@command_options.fetch(command_name, -> {}).call @option_parser, @options
      end

      def build_option_parser
        OptionParser.new do |opts|
          opts.on('-h', '--help', 'Show this help.') do
            puts opts
            exit
          end
        end
      end
  end

  class DevCache < Command
    options_for :dev_cache do |opts, _|
      opts.banner 'Toggle development mode caching on/off'
    end

    def dev_cache
      if File.exist? 'tmp/caching-dev.txt'
        File.delete 'tmp/caching-dev.txt'
        puts 'Development mode is no longer being cached.'
      else
        FileUtils.touch 'tmp/caching-dev.txt'
        puts 'Development mode is now being cached.'
      end

      FileUtils.touch 'tmp/restart.txt'
    end
  end
end

That aught to do for the documentation.

Member

kaspth commented Sep 17, 2015

Oh yeah, we'd definitely want the documentation on the commands themselves.

So for the tasks that are already run with rails we don't have to do anything about the documentation. And we can just follow their example when porting over the rake tasks.

The rails commands just rely on OptionParser within themselves and then there's the Rails command line guide for anything more advanced documentation.

So the commands should only be able to extend the option parser:

module Rails
  class Command
    def initialize(argv)
      @argv = argv

      @option_parser = build_option_parser
      @options = {}
    end

    def run(task_name)
      command_name = command_name_for(task_name)

      parse_options_for(command_name)        
      @option_parser.parse! @argv

      if command = command_for(command_name)
        command.public_send(command_name)
      else
        puts @option_parser
      end
    end

    def self.options_for(command_name, &options_to_parse)
      @@command_options[command_name] = options_to_parse
    end

    private
      @@command_options = {}

      def parse_options_for(command_name)
        @@command_options.fetch(command_name, -> {}).call @option_parser, @options
      end

      def build_option_parser
        OptionParser.new do |opts|
          opts.on('-h', '--help', 'Show this help.') do
            puts opts
            exit
          end
        end
      end
  end

  class DevCache < Command
    options_for :dev_cache do |opts, _|
      opts.banner 'Toggle development mode caching on/off'
    end

    def dev_cache
      if File.exist? 'tmp/caching-dev.txt'
        File.delete 'tmp/caching-dev.txt'
        puts 'Development mode is no longer being cached.'
      else
        FileUtils.touch 'tmp/caching-dev.txt'
        puts 'Development mode is now being cached.'
      end

      FileUtils.touch 'tmp/restart.txt'
    end
  end
end

That aught to do for the documentation.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 17, 2015

Member

By the way, I don't think @@command_wrappers is the right terminology. It's just commands. Take the rake tasks we're porting over, the command subclass will be the only place where the implementation is.

Member

kaspth commented Sep 17, 2015

By the way, I don't think @@command_wrappers is the right terminology. It's just commands. Take the rake tasks we're porting over, the command subclass will be the only place where the implementation is.

@kaspth

View changes

railties/lib/rails/commands/assets.rb
+ # rails assets:environment
+ # rails assets:precompile
+ class Assets < Command
+ def assets_clean

This comment has been minimized.

@kaspth

kaspth Sep 17, 2015

Member

I believe sprockets rails handles these, so we shouldn't add them.

@kaspth

kaspth Sep 17, 2015

Member

I believe sprockets rails handles these, so we shouldn't add them.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Sep 17, 2015

Contributor

@kaspth Cool! I'll follow that model for documentation. Thanks for the assist.

Contributor

ccallebs commented Sep 17, 2015

@kaspth Cool! I'll follow that model for documentation. Thanks for the assist.

@kaspth

View changes

railties/lib/rails/commands/test.rb
+ # rails test:db
+ class Test < Command
+ def test
+ system("bin/rake test")

This comment has been minimized.

@kaspth

kaspth Sep 17, 2015

Member

The idea is to move away from Rake, so this is going the wrong way (sample for demonstration or not).

@kaspth

kaspth Sep 17, 2015

Member

The idea is to move away from Rake, so this is going the wrong way (sample for demonstration or not).

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 17, 2015

Member

A lot of the rake tasks only have a desc, so we could have a banner shorthand:

set_banner :dev_cache, 'Toggle development mode caching on/off'
def dev_cache
  # ...
end

def self.set_banner(command_name, banner)
  options_for(command_name) { |opts, _| opts.banner banner }
end
Member

kaspth commented Sep 17, 2015

A lot of the rake tasks only have a desc, so we could have a banner shorthand:

set_banner :dev_cache, 'Toggle development mode caching on/off'
def dev_cache
  # ...
end

def self.set_banner(command_name, banner)
  options_for(command_name) { |opts, _| opts.banner banner }
end
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Sep 18, 2015

Member

The idea is to move away from Rake, so this is going the wrong way (sample for demonstration or not).

Yes, I think we should approach this in another way. Like we did for the rails test runner. Everything should be commands, outside rake, and just have the rake tasks calling the rails commands for backward compatibility.

Member

arthurnn commented Sep 18, 2015

The idea is to move away from Rake, so this is going the wrong way (sample for demonstration or not).

Yes, I think we should approach this in another way. Like we did for the rails test runner. Everything should be commands, outside rake, and just have the rake tasks calling the rails commands for backward compatibility.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Oct 19, 2015

Contributor

@kaspth Alright, so I'm awful for letting this slide. 👎 I'm going to put some serious work into it this week.

Contributor

ccallebs commented Oct 19, 2015

@kaspth Alright, so I'm awful for letting this slide. 👎 I'm going to put some serious work into it this week.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Oct 20, 2015

Member

No worries 😁

Member

kaspth commented Oct 20, 2015

No worries 😁

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Nov 11, 2015

Member

@ccallebs are you still interested in pursuing this? If not, let me know and I can swing it, otherwise great! Would love to have this in Rails 5 😁

Member

kaspth commented Nov 11, 2015

@ccallebs are you still interested in pursuing this? If not, let me know and I can swing it, otherwise great! Would love to have this in Rails 5 😁

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Nov 11, 2015

Contributor

@kaspth Ah, yes! I've put work into it but progress has stalled. Let me clean up my local tonight, put in a couple more hours, and we can see where it's at. If it's close, I'll wrap it up in the next couple of days. If it's not, feel free to take over. Sorry for going radio silent, a couple of days turns into a couple of weeks.

Contributor

ccallebs commented Nov 11, 2015

@kaspth Ah, yes! I've put work into it but progress has stalled. Let me clean up my local tonight, put in a couple more hours, and we can see where it's at. If it's close, I'll wrap it up in the next couple of days. If it's not, feel free to take over. Sorry for going radio silent, a couple of days turns into a couple of weeks.

ccallebs added some commits Aug 16, 2015

Implement new command architecture.
Add Command.rake_delegate method and convert commands.

Add simple command documentation framework.

Use Command.set_banner method for quick documentation.

Remove tasks/dev.rake.

Fully encapsulate rake --tasks commands.
@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Nov 12, 2015

Contributor

@kaspth Got the remaining commands added (all rake --tasks commands minus assets, as someone suggested I take those out). I've made a few meager attempts at testing this but I'm not sure what they would look like. Do you have any suggestions on that front?

Contributor

ccallebs commented Nov 12, 2015

@kaspth Got the remaining commands added (all rake --tasks commands minus assets, as someone suggested I take those out). I've made a few meager attempts at testing this but I'm not sure what they would look like. Do you have any suggestions on that front?

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Nov 12, 2015

Member

Sorry, I see now that rake_delegate was a bad idea. As @arthurnn said, we should be moving away from rake tasks by turning them into true Command subclasses.

I thought rake_delegate was a good idea, but it's churning a lot of our git history without being the result we want in the end. The changeset we have now changes too many things at once.

Lets scope this to the dev_cache command and the infrastructure to make that happen. Then we can follow that up with other more focused pull requests to port the others over.

Thanks again ❤️

Member

kaspth commented Nov 12, 2015

Sorry, I see now that rake_delegate was a bad idea. As @arthurnn said, we should be moving away from rake tasks by turning them into true Command subclasses.

I thought rake_delegate was a good idea, but it's churning a lot of our git history without being the result we want in the end. The changeset we have now changes too many things at once.

Lets scope this to the dev_cache command and the infrastructure to make that happen. Then we can follow that up with other more focused pull requests to port the others over.

Thanks again ❤️

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Nov 12, 2015

Contributor

@kaspth No problemo, I'll create a new branch / PR and isolate the dev_cache changes.

Contributor

ccallebs commented Nov 12, 2015

@kaspth No problemo, I'll create a new branch / PR and isolate the dev_cache changes.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Nov 22, 2015

Member

@ccallebs how's it going with isolating the dev_cache changes plus command infrastructure? 😄

Member

kaspth commented Nov 22, 2015

@ccallebs how's it going with isolating the dev_cache changes plus command infrastructure? 😄

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Nov 30, 2015

Contributor

@kaspth New PR created!

Contributor

ccallebs commented Nov 30, 2015

@kaspth New PR created!

@ccallebs ccallebs closed this Nov 30, 2015

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment