Skip to content
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

Move cakephp-codesniffer into require-dev, use CakePHP standard #12

Merged
merged 3 commits into from
Dec 26, 2014

Conversation

bcrowe
Copy link
Contributor

@bcrowe bcrowe commented Dec 25, 2014

No description provided.

@@ -35,7 +35,6 @@ before_script:
- sh -c "if [ '$INSTALL_DEPS' = '1' ]; then composer install --no-interaction --dev; fi"
- sh -c "if [ '$DB' = 'mysql' ]; then mysql -e 'CREATE DATABASE cakephp_test;'; fi"
- sh -c "if [ '$DB' = 'pgsql' ]; then psql -c 'CREATE DATABASE cakephp_test;' -U postgres; fi"
- sh -c "if [ '$RUN_CS' = '1' ]; then composer require 'squizlabs/php_codesniffer=*'; fi"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed the INSTALL_DEPS for the CS build. Fixed.

@@ -21,21 +21,16 @@ matrix:

include:
- php: 5.4
env: RUN_CS=1 RUN_TESTS=0 INSTALL_DEPS=0
env: RUN_CS=1 RUN_TESTS=0 INSTALL_DEPS=1
Copy link
Member

Choose a reason for hiding this comment

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

INSTALL_DEPS is always 1 now - so it doesn't need overriding (or doesn't need defining at all, since every permutation runs composer install).

Doing things this way will mean installing CakePHP and PHPUnit for the phpcs run, for no real reason ... however, unless someone else complains about that, looks good to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I yanked the INSTALL_DEPS env vars.

I don't think having CakePHP/PHPUnit install for the CS build is a big deal; as there's 10 other builds, everything is under a minute, and the builds will be running side-by-side anyways.

I think the real purpose of this, is that we are putting the actual required dev packages available for --dev installs. I suppose it could be kept as global just for Travis anyways (omitting it for CS build), but, again don't think it'll add any time to the complete build matrix anyways. shrug

Copy link
Member

Choose a reason for hiding this comment

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

Coolio.

It's irritating when github is being derp and the phpcs run is delayed or aborts downloading dependencies that aren't required - especially chunks of phpunit which is ofc huge. That was the original motivation for not installing dependencies for the phpcs run (though at that time phpcs wasn't a dependency, so it really was of no benefit).

Speaking with jose I think we've collectively decided to require-dev our cs standard and phpunit on all projects (except app) now.

@markstory markstory added this to the 1.0.0 milestone Dec 26, 2014
markstory added a commit that referenced this pull request Dec 26, 2014
Move cakephp-codesniffer into require-dev, use CakePHP standard
@markstory markstory merged commit 06c3662 into cakephp:master Dec 26, 2014
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.

None yet

3 participants