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

Fix tests and also add phpdoc tags / submenu method completion #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Langmans
Copy link

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)

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

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

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

@anlutro
Copy link
Owner

anlutro commented Sep 19, 2017

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?

@Langmans
Copy link
Author

  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..

@anlutro
Copy link
Owner

anlutro commented Sep 20, 2017

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.

@Langmans
Copy link
Author

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?

@anlutro
Copy link
Owner

anlutro commented Sep 21, 2017

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.

@Langmans
Copy link
Author

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

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.

unable tun tests on phpunit 5+
2 participants