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.

Sign up for GitHub

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

Await branch: fixes and minor changes #56

Open
wants to merge 2 commits into
base: await-generator-support
Choose a base branch
from
Open

Await branch: fixes and minor changes #56

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36654b1
Fixed missing import of Generator class
matcracker Mar 30, 2021
c002d66
Direct return the yield value instead of assignment
matcracker Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions libasynql/src/poggit/libasynql/base/DataConnectorImpl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

use Error;
use Exception;
use Generator;
use InvalidArgumentException;
use pocketmine\plugin\Plugin;
use pocketmine\utils\Terminal;
Expand Down Expand Up @@ -151,8 +152,8 @@ public function asyncChange(string $queryName, array $args = []) : Generator{
$onSuccess = yield Await::RESOLVE;
$onError = yield Await::REJECT;
$this->executeChange($queryName, $args, $onSuccess, $onError);
$affectedRows = yield Await::ONCE;
return $affectedRows;
//Return $affectedRows
return yield Await::ONCE;
}
Copy link
Member

Choose a reason for hiding this comment

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

since it's two lines anyway, why not make it self-explanatory instead of using a separate comment?


public function executeInsert(string $queryName, array $args = [], ?callable $onInserted = null, ?callable $onError = null) : void{
Expand All @@ -177,8 +178,8 @@ public function asyncInsert(string $queryName, array $args = []) : Generator{
$this->executeInsert($queryName, $args, static function(int $insertId, int $affectedRows) use($onSuccess) : void{
$onSuccess([$insertId, $affectedRows]);
}, $onError);
$affectedRows = yield Await::ONCE;
return $affectedRows;
//Return $affectedRows
return yield Await::ONCE;
}

public function executeSelect(string $queryName, array $args = [], ?callable $onSelect = null, ?callable $onError = null) : void{
Expand All @@ -200,11 +201,11 @@ public function executeSelectRaw(string $query, array $args = [], ?callable $onS
public function asyncSelect(string $queryName, array $args = []) : Generator{
$onSuccess = yield Await::RESOLVE;
$onError = yield Await::REJECT;
$this->executeSelect($queryName, $args, static function(array $rows, SqlColumnInfo $columns) use($onSuccess) : void{
$this->executeSelect($queryName, $args, static function(array $rows) use($onSuccess) : void{
Copy link
Member Author

@matcracker matcracker Mar 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I removed $columns parameter because if you want to use it, you should use asyncSelectWithInfo(). I forgot to write this change in the description

Copy link
Member

Choose a reason for hiding this comment

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

I added the $columns parameter because I considered it as a bad practice to create a closure that accepts fewer parameters than it will be passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so what would be the purpose of passing two parameters when only one will be used? This forces the user to needlessly pass a second parameter in my opinion

Copy link
Member

Choose a reason for hiding this comment

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

This does not force the user to pass a second parameter, because we are the user here. The definition of executeSelect stated clearly that two parameters will be passed, and only accepting one parameter is an antipattern in my opinion because that is not the same function signature.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this is not "passing" two parameters. This is "accepting" parameters.

By the way, in await-generator 4.0.0, you cannot pass yield Await::RESOLVE to callers that pass more than one parameter, because that most likely implies a bug where you forgot to handle the remaining parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Relying on the fact that passing two parameters automatically gets truncated into one parameter would cause forward compatibility problems. For example, if the caller changes to only passing one parameter (a different one), this is a BC break that should have warranted a crash, but since you always only accepted one parameter, you silently accepted the BC break without getting a crash, and hence lead to further incorrect behaviour that might damage your logic. This is dangerous.

Copy link
Member

Choose a reason for hiding this comment

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

I don't force users to always accept two arguments, but my code style requires that closures take as many parameters as they are required to, instead of silently truncating.

$onSuccess($rows);
}, $onError);
$rows = yield Await::ONCE;
return $rows;
//Return $rows
return yield Await::ONCE;
}

public function asyncSelectWithInfo(string $queryName, array $args = []) : Generator{
Expand All @@ -213,8 +214,8 @@ public function asyncSelectWithInfo(string $queryName, array $args = []) : Gener
$this->executeInsert($queryName, $args, static function(array $rows, SqlColumnInfo $columns) use($onSuccess) : void{
$onSuccess([$rows, $columns]);
}, $onError);
$rows = yield Await::ONCE;
return $rows;
//Return $rows
return yield Await::ONCE;
}

private function executeImpl(string $queryName, array $args, int $mode, callable $handler, ?callable $onError) : void{
Expand Down