Conversation
|
LGTM 👍 |
composer.json
Outdated
There was a problem hiding this comment.
Not sure if we ever reached a consensus on this..
Personally, I think it makes sense to explicitly define all dependencies - however this is something we've discussed in the past and have chosen to not do this for the other components.
As such, I'd recommend to keep this for a separate PR/discussion so that we can get this PR in earlier and am 👎 (on only this line) for now.
There was a problem hiding this comment.
Super curious why this would be omitted. It reduces friction for new contributors and is ignored when this repository is consumed as a dependency...
There was a problem hiding this comment.
The closest I could find is this: reactphp/child-process#5 (comment)
IMHO this is still worth discussing, however this is somewhat unrelated to your other changes. I'd vote to open a (meta) issue on reactphp/react so that we can discuss and apply this for all components.
There was a problem hiding this comment.
👍 on discussing it as I agree with @assertchris that it would make it easier for anyone to jump in
|
Ok, I've removed PHPUnit as a dev dependency (same as in reactphp-legacy/whois#9). |
|
@WyriHaximus ping |
|
+1 |
|
@assertchris I have no powers in this place so pinging @cboden and @clue for you |
|
Just pinging here to see if anybody with merge rights wants to settle this... |
There was a problem hiding this comment.
This LGTM (IMO) 👍
Note however that removing this condition means we can no longer run the tests if this is installed as a composer dependency.
Personally, I would consider this a non-issue, but this has been raised before: reactphp/dns#24 (comment)
There was a problem hiding this comment.
Whats the reason to change this from private to protected?
No description provided.