Skip to content

Conversation

@NigelGreenway
Copy link
Contributor

Adds:

  • ReactPHP server
  • Test suite
  • Docker integration

Copy link

@jedkirby jedkirby left a comment

Choose a reason for hiding this comment

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

So far so good, will give it a test at lunch today to see if the readme instructions are ok.

Copy link
Member

@steadweb steadweb left a comment

Choose a reason for hiding this comment

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

Minor comments added, mainly semenatics.


class WebTestCase extends TestCase
{
/** @var App $app */
Copy link
Member

Choose a reason for hiding this comment

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

Personally I don't like this one liners, I'd prefer:

/**
 * @var App $app
 */

Choose a reason for hiding this comment

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

I'm of the same preference tbh


/**
* Set up the app and client instance
* @inheritdoc
Copy link
Member

Choose a reason for hiding this comment

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

Remove.


final class ExtensionToContentTypeTest extends TestCase
{

Copy link
Member

Choose a reason for hiding this comment

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

Remove.

@@ -0,0 +1,37 @@
<?php namespace Test\Unit\Utility;
Copy link
Member

Choose a reason for hiding this comment

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

Namespace on a new line.

"description": "A webserver ran by ReactPHP",
"type": "project",
"license": "MIT",
"authors": [
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is necessary, as we have this information in git.

}
],
"minimum-stability": "stable",
"require": {
Copy link
Member

Choose a reason for hiding this comment

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

Must require php: ^7.1

use Psr\Http\Message\ServerRequestInterface as Request;
use Psr\Http\Message\ResponseInterface as Response;


Copy link
Member

Choose a reason for hiding this comment

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

Remove.


$configuration = require APP_ROUTE . 'src/Config/main.php';

$app = new Slim(
Copy link
Member

Choose a reason for hiding this comment

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

Online would read better here

$app = new Slim(new Contianer($configuration));

use Slim\Http\Response as SlimResponse;
use Slim\Http\Headers;


Copy link
Member

Choose a reason for hiding this comment

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

Remove.

{
/** @var App $app */
protected $app;
/** @var Client $client */
Copy link
Member

Choose a reason for hiding this comment

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

Add new line between properties.

@steadweb steadweb changed the title Feature/initial structure [WIP] Feature/initial structure Oct 28, 2017
@steadweb steadweb changed the title [WIP] Feature/initial structure Feature/initial structure Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants