-
Notifications
You must be signed in to change notification settings - Fork 79
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
Feature (reduce) Accumulator for multiple options of same name #93
Feature (reduce) Accumulator for multiple options of same name #93
Conversation
- Add `Option::reduce` to take an accumulator function that will be called on every token of the same name in the token list. - Add `Option::hasReducer` to check that an Option has a reducer assigned. - Update parse function for the case where a named token has an Option with a reducer, and the token has a value preceding it. The accumulator with call a `map` if also assigned on the token value before calling the accumulator with the previous accumulated value and the token value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge the latest version of dev into this and it should be mostly good to go.
src/Commando/Command.php
Outdated
@@ -486,11 +545,26 @@ public function parse() | |||
// Used in the \Iterator implementation | |||
$this->sorted_keys = array_keys($this->options); | |||
natsort($this->sorted_keys); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is still here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I based my branch off of master, but I am merging to the dev branch in an attempt to get it up to date again, so people will actually use it for PRs, as the README suggests.
Anyway. This commented out section of code exists in master, but not in dev, and is not actually part of the changes related to reduce
* @param mixed $value | ||
* @return Option | ||
*/ | ||
public function reduce($accumulator, $value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to include an optional initial value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. At the time of writing I assumed this would be another use of default()
, and it will get the default set using default()
to seed the initial value of the accumulator.
I could ad an optional $default
or $seed
param on Command::_reduce
, but I think I would implement it in a way that would just set the default value using default()
anyway.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nategood Done.
Set the default option value to value of `$seed` if supplied. This will be the default value of the accumulator on first execution.
Update function signature with optional `$seed` and include info in description.
Add
Option::reduce
to take an accumulator function that will becalled on every token of the same name in the token list.
Add
Option::hasReducer
to check that an Option has a reducerassigned.
Update parse function for the case where a named token has an
Option with a reducer, and the token has a value preceding it.
The accumulator with call a
map
if also assigned on the tokenvalue before calling the accumulator with the previous accumulated
value and the token value.