TIP: Use Markdown or, <pre> for multi line code blocks / <code> for inline code.
These forums are read-only and for archival purposes only!
Please join our new forums at discourse.kohanaframework.org
Refactoring minion module
  • The current version of the module has many mistakes and bugs, so I decided to remake the module.

    Major changes:

    • Uses MVC pattern - current task it's model + controller. Move code for show description in Controller_CLI_Manual. Delete validation from controller (use model for this).
    • Creates compact & clear helpers.
    • Rename module as CLI - it's understandable for users.

    Is now fully ready classes CLI (helper) and CLI_Exception.

    TODO:

    • Helper for work with (execute, description, convert path to class) CLI controllers. !!! Help me find name for class .
    • Basic controllers: Controller_CLI and Controller_CLI_Template.
    • Helper controllers: Controller_CLI_List (show list of CLI controllers), Controller_CLI_Manual (show description for controllers), Controller_CLI_Scaffold (create blank basic controllers).

    https://github.com/WinterSilence/kohana-cli/

  • Looks interesting. I'm a happy user of Minion — I must admit that it serves such a simple purpose that I haven't really come up against any mistakes that are annoying me enough to want to rewrite anything to fit a different CLI module. Although, I like the simpler name!

    It looks rather similar to how things were done before Minion, when a Controller's Action could be called from the command line. With the addition of useful helpers, of course.

    Your Controller_CLI has before() and after() methods, but I take it that it's designed to only run the single execute() method? In which case, why have the former two?

    Anyway, thanks for sharing! :)

  • @samwilson Controller_CLI was in TODO list, added first version of basic controllers: Controller_CLI, Controller_CLI_Template

    Update: added basic controllers, helper CLI_Manager (contains methods for working with CLI controllers).

  • I'm still not really sure what you think the problems are with Minion. Why reinvent something that's working?

  • @samwilson The current version of the minion module has some serious minuses:

    • Task not correspond to MVC and contain some methods that are better put in separate classes (static helpers, help info). In my version Minion_Task renamed in Controller_CLI - this makes it easier to understanding. Static helper methods moved in class CLI_Manager, Help info methods _help(), _parse_doccomment(), _compile_task_list() moved in Controller_CLI_List and Controller_CLI_Manual (later move them in CLI_Manager or will create an additional model \ helper class).
    • Minion_CLI contains methods that are not used (password, write_replace, wait). My version
    • Exceptions incorrectly returns an error message, need use fwrite(STDERR, $text). My version
  • Task don't correspond with MVC because they're run in a terminal.. they wouldn't need views in my opinion.

    However, I do think that if you can change things up and offer additional features over minion I'd say go for it ^^

  • @happyDemon MVC does not always mean output content, for example Controller_CLI_Scaffold use View for create controller file. Also has a number of tasks requiring data output, for example Task_Help

  • Task not correspond to MVC

    It is MVC. You shouldn't use "controllers" because they contain http crap that has nothing to do with the command line.

  • @zombor where you found "http crap " in my Controller_CLI?

  • I didn't look. I was responding to your assertion that "tasks aren't mvc". If all you did was rename tasks to controllers, there's really no point. We specifically named them something NOT controller to avoid confusion between the two. Controllers should delegated to web stuff.

  • @WinterSilence: Minion_CLI contains methods that are not used (password, write_replace, wait).

    Those methods are for the use of others' tasks I think, not Minion itself, so it's okay that they're not used anywhere.

  • @zombor "If all you did was rename tasks to controllers, there's really no point." - "if"? wtf?! as you can argue if you were too lazy to even open code))

    @samwilson I mean that these methods are not the minimum necessary. If they really someone actively uses can include them in a separate helper - so it will load and run faster.

  • wtf?! as you can argue if you were too lazy to even open code))

    Look, all I'm doing is responding to what you are saying here. It doesn't bother me one bit if you write your own CLI code, in fact I encourage it. The only thing I did was explain why we did Minion like we did.

  • Update:

    • replace `Controller` to `Task` and helpers class names
    • fix small bugs

    @zombor I did't understand why in the minion module is not amended as described above: output errors and tasks text, why each task included static\help methods. Сan at least partially transfer completion of my module to minion.

  • @WinterSilence, I reckon you should break these ideas down to their smallest chunks, and send pull requests. Then we can all see exactly what you're driving at without getting confused with a complete rewrite.

    For example, I think a Minion_CLI::error($text) would be useful (but should perhaps be a proxy for a modified Minion_CLI::write($text, $output_stream)), but I'm not going to rewrite my applications to use a new CLI module, just for the benefit of having that function. :-)

  • @samwilson Currently accumulated a lot of requests, but the answers to them are still not given. So I decided to simplify the problem and collect all fixes in new module.

    My module is not unique, and uses minion as a base. The basic concept of implementation, rather than the names of classes and methods. In my opinion, the debate on the semantic component of the code is not so important. I already partially explained the meaning changes in names, moreover it allows users to now use both modules without any conflicts of names and directories.

    I dont understand why you need add $output_stream in Minion_CLI::write($text, $output_stream) - php cli already have stream constants for this or you want add protected method Minion_CLI::_write($text, $output_stream) and two wrapper-methods Minion_CLI::write($text) and Minion_CLI::error($text) for him?.

  • Yeah, it does look like no one's very interested in merging any of the PRs to Minion doesn't it? It's a shame, because it rather makes people lose interest.

    And I didn't explain myself very well with regard to write(), sorry! What I meant was something like what you suggest:

     public static function error($text = '') {
       Minion_CLI::write($text, STDERR);
     }
     public static function write($text = '', $handle=FALSE) {
       if (is_array($text)) {
         foreach ($text as $line) {
           Minion_CLI::write($line, $handle);
         }
       }
       else {
         if (!$handle) $handle = STDOUT;
         fwrite($handle, $text.PHP_EOL);
       }
     }
    
  • sorry, my english does not allow to understand phrase: "it does look like no one's very interested in merging any of the PRs to Minion doesn't it?" :(
    Shorter and clearer:

    public static function write($text = '', $handler = STDOUT)
    {
        if (is_array($text))
        {
            $text = implode(PHP_EOL, $text);
        }
        fwrite($handler, $text);
    }
    

    update module https://github.com/WinterSilence/kohana-cli/blob/master/classes/Kohana/CLI.php#L200

  • ah, sorry! :)

    What I meant was: "No one is interested in merging the PRs."

  • @samwilson No matter how significant inquiry, most importantly its relevance. Members see that are not added to even small changes and so dont even try to make major adjustments.

  • sended "fatty" PR to minion 3.4: https://github.com/kohana/minion/pull/92
    mainly corrections documentation, there are some changes in the code, but not as global as in my module - corrected only obvious shortcomings.

    Admins! please see Add Minion module in Redmine.

Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

In this Discussion