Conversation
Bilge
force-pushed
the
v1
branch
8 times, most recently
from
0ffd9fd to
6051009
Compare
Upgraded PHP-CS-Fixer v2 -> v3. Removed position_after_functions_and_oop_constructs=>same CS rule (reverting to PSR-2). Added some return types.
Fixed IntegrationTest.
| "psr_autoloading" => true, | ||
| "return_type_declaration" => ["space_before" => "none"], | ||
| "short_scalar_cast" => true, | ||
| "single_blank_line_before_namespace" => true, |
There was a problem hiding this comment.
This should use our shared config in amphp/php-cs-fixer-config:^2, see https://github.com/amphp/amp/blob/d048ec1d03d47fc313d630e989b7a73053f10fae/.php-cs-fixer.dist.php
There was a problem hiding this comment.
Be that as it may, I do not think this issue needs to hold up this PR, nor even the 1.0 release, since styles can just be fixed after the fact.
| /** | ||
| * @var System $systemStats | ||
| */ |
There was a problem hiding this comment.
We shouldn't need such comments any longer, because we use proper types instead of generics inside promises.
| <?xml version="1.0" encoding="UTF-8"?> | ||
|
|
||
| <phpunit bootstrap="./vendor/autoload.php" colors="true"> | ||
| <phpunit colors="true"> |
There was a problem hiding this comment.
Is this now automatically included?
There was a problem hiding this comment.
I don't recall a time when it was not. PHPUnit should be launched with bin/phpunit. If you invoke it correctly, it will always include the autoloader. Perhaps if you're one of these people that thinks installing PHPUnit as a global PHAR is a good idea then it might not, but even that is probably no longer the case these days.
|
|
||
| /** @var string */ | ||
| private $tube; | ||
| private ?string $tube; |
There was a problem hiding this comment.
I don't know. This is your library, after all. I was hoping I would not need to work on it any more and you could just commit directly to this branch if you want to make changes. I'm not going to protest any change you want to make so just go ahead.
| } | ||
|
|
||
| public function put(string $payload, int $timeout = 60, int $delay = 0, $priority = 0): Promise { | ||
| public function put(string $payload, int $timeout = 60, int $delay = 0, $priority = 0) |
There was a problem hiding this comment.
We should add return types to all of these.
There was a problem hiding this comment.
I don't know what the return types are. In most cases it seems they would just be mixed anyway. But in principal I agree, as long as we're absolutely sure what the types are and that they're invariant.
2 participants
To be released as 🏷️ 1.0.0. Obviates #35.
📝 Running the examples seems to throw exceptions on connection close. This is somehow not captured by any of the tests (which are all passing).