Await branch: fixes and minor changes #56
Conversation
| @@ -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{ | |||
There was a problem hiding this comment.
I removed $columns parameter because if you want to use it, you should use asyncSelectWithInfo(). I forgot to write this change in the description
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| $affectedRows = yield Await::ONCE; | ||
| return $affectedRows; | ||
| //Return $affectedRows | ||
| return yield Await::ONCE; | ||
| } |
There was a problem hiding this comment.
since it's two lines anyway, why not make it self-explanatory instead of using a separate comment?
I was able to start the plugin with these changes but I get errors from the await library. I am attaching the crash-log. I am not very up to date on the latest changes to the await-generator library, could you help me to fix them?