Skip to content

Conversation

@WyriHaximus
Copy link
Member

No description provided.

composer.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why this is necessary.

It looks like it was originally added to the main react repository because of HHVM compatibility issues with PHAR, but that should be fixed now: reactphp/reactphp#250 (comment)

Assuming the HHVM compatibility issue is moot, see FriendsOfSymfony/friendsofsymfony.github.io#39 for prior arguments on why I'd be against listing phpunit as a dev dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

After a chat on IRC with @cboden and @clue, they seemt positive, I've made this change to the existing PR. (And the other --coverage-text PR's I've made yesterday.) Pulling it in line with @psychoticmeow's PR's like https://github.com/reactphp/socket-client/pull/8.

In my opinion composer install should install all PHP packages required to develop a package, including phpunit. Not everyone may have it installed, but more likely installs can be outdated and pulling it in likes this ensures everyone has the correct version and can run the tests.

@cboden
Copy link
Member

cboden commented Jun 14, 2014

After reading the other issues I'm actually -1 on the PHPUnit as a require-dev:

  • TravisCI already has PHPUnit
  • You can install composer globally via composer global require
  • If you're someone who runs tests you probably already have PHPUnit
  • Given the above it's consumption of resources to install per-repo

+1 to coverage-text though

@WyriHaximus
Copy link
Member Author

Ok cool, will update it Monday when I get back home 👍

cboden added a commit that referenced this pull request Jun 21, 2014
Show test coverage directly after running test
@cboden cboden merged commit 6e2578c into reactphp:master Jun 21, 2014
@clue clue mentioned this pull request Apr 17, 2015
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.

3 participants