Improve HTTP Client structure #1
Conversation
mathleite
force-pushed
the
feature/improve-http-client-structure
branch
2 times, most recently
from
f57be43 to
b5c653f
Compare
mathleite
force-pushed
the
feature/improve-http-client-structure
branch
2 times, most recently
from
7deb814 to
d537479
Compare
mathleite
force-pushed
the
feature/improve-http-client-structure
branch
from
d537479 to
129255f
Compare
|
|
||
| use Psr\Http\Message\ResponseInterface; | ||
|
|
||
| interface HttpClientAdapterInterface |
There was a problem hiding this comment.
Not sure if Adapter in the name is really necessary - (this is not a change request)
| <?php | ||
|
|
||
|
|
||
| namespace App\Notification\Client; |
There was a problem hiding this comment.
It would be more appropriated to have HttpClient instead of just Client as subdomain, because Client alone doesn't mean much, opposed as HttpClient
There was a problem hiding this comment.
Otherwise will leave the impression those are NotificationClients instead
|
|
||
| final class SlackNotification implements AppNotificationInterface | ||
| { | ||
| private SlackStylizedMessageCreator $message; | ||
| private HTTPClientAdapterInterface $client; | ||
| private HttpClientAdapterInterface $client; |
There was a problem hiding this comment.
| private HttpClientAdapterInterface $client; | |
| private HttpClientAdapterInterface $httpClient; |
|
|
||
| public function __construct(HTTPClientAdapterInterface $client, string $message, string $messageType) | ||
| public function __construct(HttpClientAdapterInterface $client, string $message, string $messageType) |
There was a problem hiding this comment.
| public function __construct(HttpClientAdapterInterface $client, string $message, string $messageType) | |
| public function __construct(HttpClientAdapterInterface $httpClient, string $message, string $messageType) |
| <?php | ||
|
|
||
|
|
||
| namespace App\Notification\Client\Guzzle; |
There was a problem hiding this comment.
| namespace App\Notification\Client\Guzzle; | |
| namespace App\Notification\HttpClient\Adapter; |
| @@ -4,12 +4,12 @@ | |||
| namespace App\Notification; | |||
|
|
|||
|
|
|||
| use App\Notification\Client\HTTPClientAdapterInterface; | |||
| use App\Notification\Client\ResponseAdapterInterface; | |||
| use App\Notification\Client\HttpClientAdapterInterface; | |||
There was a problem hiding this comment.
This namespace doesn't match the file structure.
| use App\Notification\Client\HttpClientAdapterInterface; | |
| use App\Notification\HttpClientAdapterInterface; |
|
|
||
| final class GuzzleHttpClient implements HttpClientAdapterInterface | ||
| { | ||
| private Client $client; |
There was a problem hiding this comment.
| private Client $client; | |
| private Client $guzzle; |
| use GuzzleHttp\Client; | ||
| use Psr\Http\Message\ResponseInterface; | ||
|
|
||
| final class GuzzleHttpClient implements HttpClientAdapterInterface |
There was a problem hiding this comment.
It seems at the moment there is no test for this class
| @@ -41,6 +41,6 @@ | |||
| "phpunit/phpunit": "8.5.*" | |||
| }, | |||
| "scripts": { | |||
| "tests": "./vendor/bin/phpunit tests --color=always --stop-on-failure", | |||
| "tests": "./vendor/bin/phpunit tests --color=always --stop-on-failure" | |||
There was a problem hiding this comment.
This change doesn't belong to the main intent of this PR. A more appropriated approach would be to split things up.:
- we put this PR on hold
- create a PR fixing the issue
- Once it got merged, we get back here, rebase and re-send it
| <?php | ||
|
|
||
|
|
||
| namespace App\Test\Notification\Fake; |
There was a problem hiding this comment.
It seems Fake is actually a "concrete" application subdomain. This may be very misleading.
An approach to get it clear up, is to have a test-subdomain called Support:
tests/
app/
...
support/
Notification/
HttpClient/
Adapter/
FakeHttpClient.php
...
and then pointing it out on composer.json:
"autoload-dev": {
"psr-4": {
"App\\Test\\": "tests/app",
"App\\Test\\Support\\": "tests/support"
}
}(perhaps "Test" should be the first term, actually - but this is for another discussion in another time)
| @@ -0,0 +1,23 @@ | |||
| <?php | |||
|
|
|||
There was a problem hiding this comment.
You need to establish a pattern/consistency regarding the application files structure - tabs vs spaces, how many, blank lines between <?php and namespace statements, etc :)
There a lot of variations it seems.
I'd suggest you take a look on https://editorconfig.org
No description provided.