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

Refactor #2

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Refactor #2

wants to merge 16 commits into from

Conversation

dvalchanov
Copy link
Member

Final changes and additions.

@vesln
Copy link

vesln commented Sep 21, 2012

Please add license and copyright to every single .php file:

/**
 * Aero.io API client for PHP
 *
 * @copyright Copyright 2012, aero.io (http://aero.io)
 * @license The MIT License
 */

@vesln
Copy link

vesln commented Sep 21, 2012

Please setup Travis CI to run the tests on each commit in master, and add a travis badge to the "Readme" file.

Make sure it will run them on PHP versions: 5.2, 5.3 and 5.4 (if available)

http://about.travis-ci.org/docs/user/languages/php/

* Engine interface.
*
* Interface that should be used by all of the supplied engines.
*/
interface Engine {
public function execute($request);
Copy link

Choose a reason for hiding this comment

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

Missing comment.

*
* @var array
*/
protected $schema = array(
Copy link

Choose a reason for hiding this comment

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

This schema is a bit incomplete, please include not only the main attributes, but all of them, except checksum and announced.

@vesln
Copy link

vesln commented Sep 21, 2012

You've used tabs instead of 4 spaces in a lot of files, for example ResponseTest.php. Please convert them to 4 spaces and configure your Vim to show you that you are using tabs instead of spaces.

@vesln
Copy link

vesln commented Sep 21, 2012

AeroConnectionTest.php is empty, either remove it or write some friggin tests ;)

@vesln
Copy link

vesln commented Sep 21, 2012

You have no tests for the path methods in the resources. Please check your test case since i find a lot of other issues there.

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.

2 participants