php-menu icon indicating copy to clipboard operation
php-menu copied to clipboard

Fix tests and also add phpdoc tags / submenu method completion

Open Langmans opened this issue 8 years ago • 7 comments
trafficstars

fixes #7

I realize submitting a PR for each item would be better, but they shouldn't break any functionality.

[x] removeItem should return true and be optimal for runtime [x] phpunit fix for newer versions [x] some phpdoc fixes for IDEs [x] magic method completion for submenus [x] fix expected output when running on windows or non-posix osx (\r\n and \r respectively)

Langmans avatar Sep 19 '17 18:09 Langmans

HHVM fails, but shouldn't composer have "phpunit/phpunit:*"?

Also, I don't see any use of mockery in your tests at the moment...

Langmans avatar Sep 19 '17 19:09 Langmans

Hey, looks good to me with a few exceptions.

The project was initially set up using tabs for indentation. Could you either stick with this, or submit a separate PR just for converting to spaces (PSR-2), then rebase this PR on top of that? For what it's worth, I think just converting to tabs will save you the headache of a difficult rebase.

It seems like it's possible for there to be two items that should be removed in the removeItem code, I think that's the purpose of not returning early - could you prove me wrong on that?

anlutro avatar Sep 19 '17 20:09 anlutro

  1. Totally forgot tabs :) Also, didn't see any editorconfig (http://editorconfig.org/)
  2. item id's are supposed to be unique, so that's why I added return statements. But it seems I used the wrong phpdoc return..

Langmans avatar Sep 20 '17 17:09 Langmans

Yeah I never got around to editorconfig or anything like that. My editors adapt to the project style automatically so I more or less assume that others do the same.

Technically this is possible:

$item = $collection->makeItem('Example', '/example');
$collection->addItemInstance($item, 10);
$collection->addItemInstance($item, 10);

In this case, removeItem would only remove one of the items. Not sure if it should count as undefined behaviour that's OK to break, though.

anlutro avatar Sep 20 '17 17:09 anlutro

Also, there is this:

$item = $collection->makeItem('Example', '/example');
$collection->addItemInstance($item, 10);
$item2 = $collection->makeItem('Example', '/example');
$collection->addItemInstance($item2, 10);

$collection->removeItem('example');
// $item2 is deleted now, but is $item1 also deleted?

Adding $item2 should actually throw an exception if you ask me, since it's already in the $items property. Or maybe it would make more sense if spl_object_hash is used for the id hash?

Langmans avatar Sep 20 '17 18:09 Langmans

Maybe this particular change should be put in a separate pull request. I think it's a somewhat difficult question and a breaking change, and I would like to merge your other changes.

anlutro avatar Sep 21 '17 06:09 anlutro

Ok, disregard this PR. I will redo the changes and submit a new PR.

Langmans avatar Sep 22 '17 11:09 Langmans