mnapoli / silly

Silly CLI micro-framework based on Symfony Console
MIT License
922 stars 52 forks source link

Default parameters for hyphen variables #34

Closed fire015 closed 7 years ago

fire015 commented 7 years ago

Arguments and options containing hyphens are matched to camelCase variables, however this does not work when specifying default parameters for those variables.

$console->command('app:import [number-of-clicks]', function ($numberOfClicks = 1) {
    var_dump($numberOfClicks); // expected 1 received null
});

This only works for an exact variable name match. Is there anything you can do about this?

mnapoli commented 7 years ago

At the moment I'm a bit swamped unfortunately, but if you or anyone else wants to send a failing test case that covers this and a bugfix in a pull request that would be awesome

nikush commented 7 years ago

I don't mind stepping in to help. I was able to create a failing test case, but struggled on the actual bug fix.

From my findings, it looks like the HyphenatedInputResolver is only used by the Invoker when calling commands, so when the command runs and hyphenated arguments/options are provided they are correctly passed through to the callable. However, when a command is added to the application and the default values looked up via reflection, the callable's arguments and the definition's arguments are paired up without any transformation happening to accommodate for hyphenated arguments, as can be seen here.

@mnapoli would you like me to submit a pull request with just the failing test case? Then this discussion for a solution could carry on there.

mnapoli commented 7 years ago

@nikush yep that would be a good starting point (that way anybody can reuse your test).

However, when a command is added to the application and the default values looked up via reflection, the callable's arguments and the definition's arguments are paired up without any transformation happening to accommodate for hyphenated arguments, as can be seen here.

Good point, maybe there should be "hyphenation" happening here (I don't know if that's a real word ^^)

mnapoli commented 7 years ago

Fixed in #35 thanks @fire015 and @nikush!