Skip to content

Conversation

@erayd
Copy link
Contributor

@erayd erayd commented Feb 21, 2017

What

  • Add php-cs-fixer to require-dev
  • Add style-check and style-fix scripts to composer

Why

  • To make style checks & fixes easy (currently, php-cs-fixer must be manually fetched and run)
  • To encourage users to run these checks locally first rather than relying on travis integration to flag errors

@erayd erayd force-pushed the composer-style-scripts branch 5 times, most recently from 86703ba to 328a299 Compare February 21, 2017 22:42
@bighappyface
Copy link
Collaborator

Thoughts on TravisCI failure?

@erayd erayd force-pushed the composer-style-scripts branch from 328a299 to 71b6fc7 Compare February 21, 2017 22:49
@erayd
Copy link
Contributor Author

erayd commented Feb 21, 2017

I've been trying to figure out how to resolve it - think I have a solution now. Have set sed to remove the require-dev dependency on php-cs-fixer for hhvm and nightly builds, because php-cs-fixer explicitly conflicts with them, and we're not trying to run it on those platforms anyway.

@erayd
Copy link
Contributor Author

erayd commented Feb 21, 2017

Yep, that worked - Travis is now happy :-).

@bighappyface
Copy link
Collaborator

Works for me. @alcohol @shmax any concerns?

Copy link
Collaborator

@bighappyface bighappyface left a comment

Choose a reason for hiding this comment

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

+1

@erayd erayd force-pushed the composer-style-scripts branch from 71b6fc7 to c69da1c Compare February 22, 2017 04:12
@erayd
Copy link
Contributor Author

erayd commented Feb 22, 2017

Rebased on master following merge of PR #349.

@alcohol
Copy link
Contributor

alcohol commented Feb 22, 2017

None at all, seems like an elegant enough solution for CI, definitely. :-)

@bighappyface bighappyface merged commit f6a9111 into jsonrainbow:master Feb 22, 2017
@erayd erayd deleted the composer-style-scripts branch February 22, 2017 12:50
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.

3 participants