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

Add easy way to use simple closures as commands #58

Closed
wants to merge 1 commit into from
Closed

Add easy way to use simple closures as commands #58

wants to merge 1 commit into from

Conversation

henrikbjorn
Copy link
Contributor

This fixed #27

I know this is a BC but since it targets 2.0 i think it is okay.

The reason for renaming command to add is that it adds the command to the application, which is the same terminology used by a normal Console Application. Also when using closures the name command feels more natural.

here is an example, the syntax should be equal to the one used in Silex with its get, post etc functions.

<?php

$cilex = new Cilex\Application;
$cilex->command('my-command-name', function ($input, $output) {
    // do something
});

$command = $cilex->command('my-command-name', function ($input, $output) {
    // do something
});

$command->addOption(); // etc.

public function command($name, $callable)
{
$command = new Command($name);
$command->setCode($callable);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't setCode only influence the execute method of a Command? Unless I got it wrong I am missing a way to add argument and option definitions and a way to set the description for a command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, thats why the created command object is returned from the function. Its a shortcut, not the "best" way to create full commands.

In silex binding would be

$app->get('/', function () {})->bind('route');

which is the same as cilex

$app->command('my-name', function () {
})->addOoption(); // etc.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting notion, I hadn't thought of that option. Awesome!

@mvriel
Copy link
Member

mvriel commented Oct 13, 2014

I would merge this feature except that I am not entirely convinced of the 'add' method name. This is because add usually implies that you add something to the collection on which you invoke it and the Pimple\Container is a Dependency Injection Container.

Hence add suggests to me that you add a service to the container.

I don't have any other alternatives at hand quickly (except for the existing command).

Let's explore the names a bit; I will try to think of new ones. Do you have more suggestions?

@henrikbjorn
Copy link
Contributor Author

maybe set i dunno.

I understand that think because Application extends pimple, but it is its own thing, and as Pimple only really expose offsetGet and offsetSet i think add() is a good enough compromise of keeping it intuiative for the end user to add a command and also not to confuse it (as you set services on a container, not add them).

@henrikbjorn
Copy link
Contributor Author

should this be closed?

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

Successfully merging this pull request may close these issues.

ClosureCommand or another easy way to add new commands
2 participants