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

container entries debug #505

Merged
merged 5 commits into from Jun 11, 2017
Merged

container entries debug #505

merged 5 commits into from Jun 11, 2017

Conversation

@juliangut
Copy link
Contributor

@juliangut juliangut commented Jun 9, 2017

covers container entries debug dumper as per comment on #152

juliangut added 2 commits Jun 9, 2017
Copy link
Member

@mnapoli mnapoli left a comment

Thanks! I have written a few comments inline, also could you add a changelog entry in the 6.0 section?

$this->assertEquals('foo', $container->debugEntry('string'));
$this->assertEquals('string', $container->debugEntry('entry'));
$this->assertEquals('stdClass', $container->debugEntry('object'));
}

This comment has been minimized.

@mnapoli

mnapoli Jun 10, 2017
Member

This test is ContainerGetTest.php and it's supposed to test the get() method.

I think it would make more sense to move those 2 new tests into a new test class in IntegrationTest (instead of UnitTest) that would test the debug features, i.e. DebugTest ?

*
* @return array
*/
public function getKnownEntryNames()

This comment has been minimized.

@mnapoli

mnapoli Jun 10, 2017
Member

You can add the array return type here and remove the same line that is in the phpdoc (PHP 7 FTW ^^)

This comment has been minimized.

@juliangut

juliangut Jun 10, 2017
Author Contributor

I think code base must be reviewed on missing type hinting hunt, for example on Container::has

This comment has been minimized.

@mnapoli

mnapoli Jun 11, 2017
Member

I cannot add it for has() and get() because those are defined in PSR-11 unfortunately.

*
* @return string
*/
public function debugEntry($name)

This comment has been minimized.

@mnapoli

mnapoli Jun 10, 2017
Member

Could you add the strict types here too? (parameter + return type)

return is_object($entry) ? get_class($entry) : gettype($entry);
}

throw new InvalidArgumentException("No entry or class found for '$name'");

This comment has been minimized.

@mnapoli

mnapoli Jun 10, 2017
Member

You could throw a NotFound exception? (it wouldn't be a huge difference but since the exception exists let's use it)

@mnapoli mnapoli added the enhancement label Jun 10, 2017
@mnapoli mnapoli added this to the 6.0 milestone Jun 10, 2017
@mnapoli
Copy link
Member

@mnapoli mnapoli commented Jun 11, 2017

Thanks!

@mnapoli mnapoli merged commit 6b55493 into PHP-DI:master Jun 11, 2017
4 checks passed
4 checks passed
Scrutinizer 2 updated code elements
Details
continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 95.052%
Details
@juliangut juliangut deleted the juliangut:container-debug branch Jun 11, 2017
@juliangut juliangut mentioned this pull request Feb 3, 2018
mnapoli added a commit that referenced this pull request Feb 3, 2018
This reverts commit 6748f3f.

The method getKnownEntryNames introduced in PR #505 has been removed as being considered "unused".

Without that method debugEntry method is sort of orphan as you may only debug entries you know exist in the container.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants