Skip to content

Conversation

@ascii-soup
Copy link
Contributor

I am keen to start contributing as I'm about to begin using this library for a project I'm working on.

I don't have phpunit installed globally and since it now supports being installed with composer, I've added it as a dev dependency to make it easier to run the tests in this project.

@clue
Copy link
Owner

clue commented Mar 3, 2016

Thanks for your PR @ascii-soup! 👍

I know this is a controversial change, but I honestly don't mind either way :) Do you happen to have an insight if a consensus has been reached in the broader PHP community?

Also, IF we are to add this, we should probably use a version which is compatible with 5.3+ through 7 and HHVM and also update our Travis script.

@ascii-soup
Copy link
Contributor Author

Good point on compatibility, I guess we could pin to a lower version of phpunit.

As for the Travis script, I thought I'd changed it! But evidently not. I can tidy up both of these things today and add to the PR.

Edit: As for the wider PHP community, I can't really say. We've certainly ditched anything that has a composer alternative, especially within our packages, since it makes contributing easier, in a "Batteries included" fashion.

@clue
Copy link
Owner

clue commented Mar 3, 2016

Thanks for your quick reply and I'm looking forward to seeing your update 👍

Good point on compatibility, I guess we could pin to a lower version of phpunit.

See also Composer's composer.json: https://github.com/composer/composer/blob/f2d606ae0c705907d8bfa1c6f884bced1255b827/composer.json#L38

@ascii-soup
Copy link
Contributor Author

Right, think this is all OK now.

@clue
Copy link
Owner

clue commented Mar 8, 2016

This should probably be added to require-dev instead of require, otherwise LGTM 👍

@ascii-soup
Copy link
Contributor Author

D'oh. That's what I get for squeezing it in between other bits of work.
Will fix up tomorrow :)
On 8 Mar 2016 22:07, "Christian Lück" [email protected] wrote:

This should probably be added to require-dev instead of require,
otherwise LGTM [image: 👍]


Reply to this email directly or view it on GitHub
#26 (comment).

@ascii-soup
Copy link
Contributor Author

OK - sorry for the delay. Added to require-dev.

@clue
Copy link
Owner

clue commented Jun 10, 2016

Thanks for your PR @ascii-soup, let's get this in already :shipit:

For the reference: The Travis build time went up from ~30s to ~50s due to this change. IMO this is acceptable 👍

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