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

Increase code coverage #98

Merged
merged 4 commits into from
Dec 6, 2016
Merged

Increase code coverage #98

merged 4 commits into from
Dec 6, 2016

Conversation

snsanich
Copy link
Contributor

@snsanich snsanich commented Dec 3, 2016

Hi.

I applied the same test suite on AbstractLazyCollection as for ArrayCollection (definitely, tests on __toString and Selectable interface was excluded for AbstractLazyCollection)

…ion (excluded tests on __toString and Selectable interface)
@Ocramius Ocramius added this to the 1.4.0 milestone Dec 4, 2016
@Ocramius Ocramius self-assigned this Dec 4, 2016
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Good improvements!

Needs:

  • whitespace cs fixes
  • usage of ::class where applicable
  • usage of short arrays

Thanks for working on this, @snsanich!

@@ -10,13 +11,20 @@
*/
class LazyArrayCollection extends AbstractLazyCollection
{
private $_onInitialization;
Copy link
Member

Choose a reason for hiding this comment

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

Docblock. Also, please don't prefix private/protected stuff with _ (we used to do it with PHP 4, when visibility wasn't supported yet)

// Intentionally empty.
};

self::assertInstanceOf('Doctrine\Tests\DerivedArrayCollection', $collection->map($closure));
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class instead of string references

self::assertContainsOnlyInstancesOf('Doctrine\Tests\DerivedArrayCollection', $collection->partition($closure));
self::assertInstanceOf('Doctrine\Tests\DerivedArrayCollection', $collection->matching(new Criteria));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Empty newline required

/**
* @author Alexander Golovnya <snsanich@gmail.com>
*/

Copy link
Member

Choose a reason for hiding this comment

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

Remove this whitespace

{
$collection = new DerivedArrayCollection(new \stdClass);
$closure = function () {
// Intentionally empty.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the why this is intentionally empty?

Copy link
Member

Choose a reason for hiding this comment

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

I assume that this should never actually be called?


public function testLazyCollection()
{
$collection = $this->_buildCollection(array('a', 'b', 'c'));
Copy link
Member

Choose a reason for hiding this comment

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

Can you switch to short arrays when you move around these files?

snsanich pushed a commit to snsanich/collections that referenced this pull request Dec 6, 2016
Requests described in doctrine#98
whitespace cs fixes
usage of ::class where applicable
usage of short arrays

Applied:
php-cs-fixer --rules=@symfony fix tests/Doctrine/Tests
@snsanich
Copy link
Contributor Author

snsanich commented Dec 6, 2016

I made necessary changes.
I can miss something, @Ocramius , if I haven't understood you correctly.

@Ocramius
Copy link
Member

Ocramius commented Dec 6, 2016

@snsanich looking good, waiting for travis :-)

snsanich pushed a commit to snsanich/collections that referenced this pull request Dec 6, 2016
Requests described in doctrine#98
whitespace cs fixes
usage of ::class where applicable
usage of short arrays

Applied:
php-cs-fixer --rules=@symfony fix tests/Doctrine/Tests
Requests described in doctrine#98
whitespace cs fixes

Applied:
php-cs-fixer --rules=@symfony fix tests/Doctrine/Tests
snsanich pushed a commit to snsanich/collections that referenced this pull request Dec 6, 2016
Requests described in doctrine#98
usage of ::class where applicable
usage of short arrays
PHP 5.3, 5.4 is not supported anymore
Requests described in doctrine#98
usage of ::class where applicable
usage of short arrays
PHP 5.3, 5.4 is not supported anymore
@snsanich
Copy link
Contributor Author

snsanich commented Dec 6, 2016

Php 5.3, 5.4 doesn't support short array syntax and Class name resolution via ::class. I removed php 5.3, 5.4 from Travis.yml

@Ocramius
Copy link
Member

Ocramius commented Dec 6, 2016

If we're not testing 5.3 and 5.4, then it should also be gone from composer.json. Can you update it there too?

We're not testing 5.3 and 5.4
@Ocramius
Copy link
Member

Ocramius commented Dec 6, 2016

@snsanich awesome, thanks!

@Ocramius Ocramius merged commit 0b0b541 into doctrine:master Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants