-
-
Notifications
You must be signed in to change notification settings - Fork 26
Update actions #38
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
Update actions #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR 👍 any sorry for the delayed response -- I've added some review comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this library not just uses PHPUnit as dependency, but is in itself also intended as an addon for PHPUnit, I'd like to keep testing against multiple versions of PHPUnit (at least those that are supported by this lib, which tend to correlate with the officially supported versions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is, to let composer decide, which PHPUnit version to use, depending on the current PHP version. It should cover most versions, without explicitly declaring them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption was that most users will have a dependency to phpunit/phpunit declared directly in the require-dev section of their composer.json, and will then require-dev any assertion libraries (like this one) on top/in addition to their existing PHPUnit requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer keeping the tags configuration, here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't make much sense Imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Care to elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's just absolutely no need to perform another test upon tag creation. Creating a tag requires a push or a merged PR and in both cases, the tests have already been performed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems unnecessarily restrictive. I can already see a maintainer in a far-distant future (probably me) wonder why a pipeline didn't run after modifying the phpunit.xml, or adding a fixture JSON file, etc. I'm fine with non-code changes triggering an extra (potentially superflous) build for the sake of simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
n/t