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

Refactor menu building process #155

Merged
merged 5 commits into from May 16, 2018
Merged

Refactor menu building process #155

merged 5 commits into from May 16, 2018

Conversation

AydinHassan
Copy link
Member

@AydinHassan AydinHassan commented May 15, 2018

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 only Builder shared by CliMenuBuilder and SplitItemBuilder. 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

  1. Code completion is fixed
  2. The code is easier to read and follow with deeper nested menus
  3. Code can be automatically aligned correctly by ide's which do not understand ->end() should deindent
  4. Simplifies builder code
  5. Removed useless interface and trait created to help builder chaining with split item menu

TODO:

  • Update examples
  • Update docs
  • Remove CliMenuBuilder class_alias - no point anymore the api has changed too much to be useful
  • Update changelog
  • Update upgrading.md

@codecov-io
Copy link

codecov-io commented May 15, 2018

Codecov Report

Merging #155 into master will increase coverage by 2.04%.
The diff coverage is 95.29%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
src/CliMenu.php 98.34% <100%> (+0.02%) 93 <2> (+2) ⬆️
src/Builder/SplitItemBuilder.php 89.28% <89.28%> (+89.28%) 7 <7> (-1) ⬇️
src/Builder/CliMenuBuilder.php 96.57% <98.11%> (+4.79%) 45 <10> (-6) ⬇️
src/MenuItem/AsciiArtItem.php 100% <0%> (ø) 29% <0%> (+13%) ⬆️
src/MenuItem/MenuMenuItem.php 100% <0%> (+13.33%) 7% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5909ec5...f637104. Read the comment docs.

@Lynesth
Copy link
Collaborator

Lynesth commented May 16, 2018

Wouldn't you prefer doing something like (using $this inside builders callback):

<?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);

@AydinHassan
Copy link
Member Author

AydinHassan commented May 16, 2018

@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 AydinHassan changed the title WIP: Refactor menu building process Refactor menu building process May 16, 2018
Copy link
Collaborator

@Lynesth Lynesth left a comment

Choose a reason for hiding this comment

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

👍

@AydinHassan AydinHassan merged commit c2adaff into master May 16, 2018
@AydinHassan AydinHassan deleted the simplify-builder branch May 16, 2018 11:44
@AydinHassan AydinHassan added this to the 3.0 milestone May 16, 2018
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.

None yet

3 participants