Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
============================================
+ Coverage 95.96% 98.01% +2.04%
+ Complexity 431 429 -2
============================================
Files 27 25 -2
Lines 1315 1311 -4
============================================
+ Hits 1262 1285 +23
+ Misses 53 26 -27
Continue to review full report at Codecov.
|
Collaborator
|
Wouldn't you prefer doing something like (using <?php
use PhpSchool\CliMenu\Builder\SplitItemBuilder;
use PhpSchool\CliMenu\CliMenu;
use PhpSchool\CliMenu\Builder\CliMenuBuilder;
require_once(__DIR__ . '/../vendor/autoload.php');
$itemCallable = function (CliMenu $menu) {
echo $menu->getSelectedItem()->getText();
};
$menu = (new CliMenuBuilder)
->setWidth(100)
->addSplitItem(function () use ($itemCallable) {
$this->addSubMenu('Sub Menu on a split item', function () {
$this->setTitle('Behold the awesomeness')
->addItem('This is awesome', function() { print 'Yes!'; })
->addSplitItem(function () {
$this->addItem('Split Item 1', function() { print 'Item 1!'; })
->addItem('Split Item 2', function() { print 'Item 2!'; })
->addItem('Split Item 3', function() { print 'Item 3!'; })
->addSubMenu('Split Item Nested Sub Menu', function () {
$this->addItem('One', function() { print 'One!'; })
->addItem('Two', function() { print 'Two!'; })
->addItem('Three', function() { print 'Three!'; });
});
});
})
->addItem('Item 2', $itemCallable)
->addStaticItem('Item 3 - Static')
->addItem('Item 4', $itemCallable);
})
->build();
$menu->open();If so you can simply replace the three $callback($builder);by $callback = $callback->bindTo($builder);
$callback();(I guess you could even allow both $this and custom variable if you want... not sure that's the best idea) $callback = $callback->bindTo($builder);
$callback($builder); |
Member
Author
|
@Lynesth not exactly - the preference is the current approach for reasons of code completion. However, I don't see why can't allow both, it is a little more streamlined with your approach and I can see why some people might prefer it, and it's not exactly much code to implement. |
AydinHassan
changed the title
This was referenced Dec 17, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ok so this is quite a large PR and completely rewrites the builder process. It helps me accomplish a few things and has a bunch of added benefits that I will go through. It is a work in progress, well, it is working and the tests are passing but the examples and docs needs to be re-written, but I would like some feedback before I go ahead and do that.
Problem
The builder code was becoming complicated with additional features. Because now sub menus can be added to menus as well as split items when using the
->end()syntax to go back down a level the code completion was broken because it hinted an instance of onlyBuildershared byCliMenuBuilderandSplitItemBuilder. The loss of code completion for me rendered the builder pretty useless.Solution
Remove nesting and
->end()calls - submenus and split items are now just configured in callbacks.Advantages
->end()should deindentTODO: