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

Refactoring/adjustments #20

Merged
merged 11 commits into from
Jul 30, 2014
Merged

Refactoring/adjustments #20

merged 11 commits into from
Jul 30, 2014

Conversation

goatherd
Copy link

Various minor fixes/ reversions:

  • no composer.lock
  • adjusted travis.yml
  • fixed utf8 issue in class comments
  • re-enabled testsuite (using new binary)
  • made standard name obligatory for binary selecting (using directory name by default)
  • adjusted readme to suggest latest PHP only
  • reverted phpcs version range

@goatherd
Copy link
Author

@reneoelke for the next steps phpcs2 is required and it is currently used by the master branch so I reverted your changes for that.

Major benefits are:

  • support for custom reports
  • support for custom CLI arguments
  • code cleanup simplifies coding against the library
  • most current contributions go against phpcs2

Otherwise there is no difference in just running the tests with the default reporting.

An other change reverted is the composer.lock. We opted against those last year and I saw no reasoning to add it here.

@goatherd
Copy link
Author

@reneoelke experimentally I tested against hhvm which currently fails. If it is trivial to fix I will keep this otherwise it will be dropped for now.

The inclusion of hashbanged phpcs from the binary may be an issue. Will test this later on.

@goatherd
Copy link
Author

@reneoelke the issue lies with tempnam() and is resolved in a pull-request.

I will temporarily mark hhvm invalid until this is fixed.

@goatherd goatherd mentioned this pull request Jul 28, 2014
8 tasks
@@ -1,2 +1,3 @@
/vendor
/phpunit.xml
/composer.lock
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason for removing the composer.lock file? It is official recommended to save this in git (see https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file "Commit your application's composer.lock (along with composer.json) into version control.").

Copy link
Author

Choose a reason for hiding this comment

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

There was no need for it and we opted against. It is recommended only for non-libraries. As we did not discuss this any further I reverted your change.

@reneoelke
Copy link
Owner

  1. Please don't use unstable (unreleased) phpcs2. For now its better to use 1.x. For the next steps we could switch to 2.x but in a special branch. Now we want to have a stable refactored master as development base for the upcoming Snout application.
  2. Please don't test anything with hhvm here. It costs too much time that is better to use for completing the application.

@goatherd
Copy link
Author

@reneoelke adjusted to use stable phpcs 1.5.3 using composer.lock updating to ~1.5.

With this the pull request is completed. Would you like to review it again?

reneoelke added a commit that referenced this pull request Jul 30, 2014
@reneoelke reneoelke merged commit b10d0eb into reneoelke:feature/refactoring Jul 30, 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

2 participants