Review parameter names in ext/pspell #6257
Conversation
ext/pspell/pspell.stub.php
Outdated
|
|
||
| function pspell_check(int $pspell, string $word): bool {} | ||
| function pspell_check(int $dictionary_id, string $word): bool {} |
There was a problem hiding this comment.
since the manual describes the return value of pspell_new() as Returns the dictionary link identifier on success or FALSE on failure., I think it makes sense to rename $pspell to $directory_id (the manual refers to it as $directory_link).
There was a problem hiding this comment.
O M G. Wow, this is even worse than usual! This extension uses resources, but instead of actually using them it works on resource IDs???
There was a problem hiding this comment.
So, considering that we should migrate these resource IDs to objects in the future, we should name them with that in mind. And we did use the $mysql, $ldap etc convention before, so keeping this at $pspell would make sense to me. $dictionary_id is sensible now, but not if we ever migrate this to a non-integer.
There was a problem hiding this comment.
I had a suspicion for a second that these are really resource IDs, but I thought that it's impossible, so rejected the idea.. Crazy!
There was a problem hiding this comment.
With that in mind, $pspell and$config seems the best choices for me as well.
kocsismate
force-pushed
the
pspell-name
branch
from
cd47c4e to
35ae42d
Compare
No description provided.