Skip to content

Update composer.json #31

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

Merged
merged 4 commits into from
Feb 16, 2018
Merged

Update composer.json #31

merged 4 commits into from
Feb 16, 2018

Conversation

Arzaroth
Copy link
Contributor

A straightforward change.
The goal of this PR is to have a php version requirement (I chose the lowest one from the CI setup) and also to install phpunit locally as a dev dependency.

@mikehaertl
Copy link
Owner

I agree to the PHP constraint but I'm not sure about phpunit. I'd rather leave this up to every developer. Some might prefer to install phpunit system wide. Actually I'm a bit confused about the require-dev section anyway. I think I remember that composer always installed everything. I may be wrong on this, though.

composer.json Outdated
@@ -9,6 +9,12 @@
"email": "haertl.mike@gmail.com"
}
],
"require": {
"php": "^5.4.0 || ^7.0"
Copy link
Owner

Choose a reason for hiding this comment

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

How about a single >=5.4.0 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did initially, CI failed for php7. You can see why in the output of job 99.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, i see a ^5.4.0, not a >=5.4.0 in your initial commit.

@mikehaertl
Copy link
Owner

Regarding require-dev, now I remember. You explicitely have to tell composer that you don't want development dependencies installed with --no-dev. I don't like that default behaviour as it means, that you get a bloated vendor directory by default. So I'd say, let's leave that phpunit dependency out.

@Arzaroth
Copy link
Contributor Author

Arzaroth commented Feb 16, 2018

From my point of view, it doesn't hurt to install phpunit locally even if it's already installed system wide.
I do agree about require-dev being confusing.
You have to run composer install with --no-dev switch to disable require-dev dependencies to be installed.
Although I'm not sure but I think require-dev dependencies are not installed when requiring a package that has them.

EDIT because of cross-post : Well yes, the vendor directory will be bloated for the local development version of this package, but not for those requiring this one.

@mikehaertl
Copy link
Owner

So far the development team is very small. I can't see why we want to bloat everyone elses vendor directory with phpunit. It's not required to use this library. And the few developers here will know how to install phpunit if they need it.

So: Sorry, I'm a bit insistent on this one. :). I have a personal aversion against software that doesn't respect my valuable disk space by default.

@Arzaroth
Copy link
Contributor Author

Duly noted, I'll leave that out of this PR :).

@mikehaertl
Copy link
Owner

Great :). Can you also give >= 5.4.0 a try? The failed test had ^5.4.0. I personally prefer this notation as it's slightly easier to read (I always have to look up the exact meaning of ^).

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.

2 participants