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

[Develop] Add travis script #304

Merged
merged 13 commits into from
Apr 22, 2015
Merged

[Develop] Add travis script #304

merged 13 commits into from
Apr 22, 2015

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Apr 22, 2015

Needs IRL test

Also:

  • Rename phpcs.xml to ruleset.xml so PHPStorm can pick it up
  • Fix all but one remaining code style issue
  • only show var_dump() on error when WP_DEBUG is set to true
  • Remove unnecessary concatenation in echo statements for lower memory usage

@jrfnl jrfnl added this to the 2.5.0 milestone Apr 22, 2015
# Might as well test HHVM too
- hhvm
# Lowest currently possible in travis
- 5.2
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't Travis only support 5.3 min now?

What about 5.3, 5.4, 5.5? Occasionally something breaks there, and currently we would only know that it passed the low PHP level, but failed at 5.6.

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'm pretty sure they reverted the removal of PHP 5.2 (under pressure of the WP community).

As we're currently only doing a lint and phpcs, running it on more versions would be wasting resources as the results won't change. If it breaks on either of those two versions, we need to look at the code, everything in between is caught by this.

Once we have unit tests, we should start running it on more versions, though still only lint and phpcs on highest and lowest.

@GaryJones
Copy link
Member

I think the addition of .travis.yml, the change in phpcs config file, and the code fixes should all have been in different branches. I like the code changes, but have questions about the other two.

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 22, 2015

I think the addition of .travis.yml, the change in phpcs config file, and the code fixes should all have been in different branches. I like the code changes, but have questions about the other two.

I'll keep that in mind for that future.

In this case I think they are related (except for the concatenation commit which was just extra). Adding the travis script which runs phpcs and will fail straight away on errors would mean that the travis config could not be merged. (providing we'll hook it in now to test this PR before merge)

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 22, 2015

Updated PR

GaryJones added a commit that referenced this pull request Apr 22, 2015
Add Travis-CI script, fix PHPCS errors
@GaryJones GaryJones merged commit 346189a into develop Apr 22, 2015
@jrfnl jrfnl deleted the develop-travis branch April 22, 2015 21:38
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 23, 2015

@GaryJones shouldn't we hook travis in first to test this PR ?

@GaryJones
Copy link
Member

It will get tested once we are hooked up - we can adjust it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants