Fix types in method signatures and docblocks #88
Conversation
Naktibalda
force-pushed
the
type-fixes
branch
3 times, most recently
from
7989fdb to
c7046b7
Compare
There was a problem hiding this comment.
Okay,
The changes that I would suggest in this PR have to do with the types of functions that are implemented from an interface.
Please verify that the signature of the methods corresponds exactly to each of the interfaces implemented in this class.
Taking a quick look I found for example that loadSessionSnapshot has a string $name parameter that is not typed yet, whereas you should remove all the : voids and : ?strings that you added in methods that implement an interface.
I am also curious about a matter. I didn't add a docblock in a function that implements an interface because I remember you once told me that doing that overwrites the documentation generated by robo. If that is the case, then you must also add the method description and the code example from the interface.
We should only change the typing of the methods that come from interfaces when we release a version that points and requires Codeception 5.
Even if I know that we are already breaking compatibility by modifying the other methods, we shouldn't mix up those changes. They must be different PR's as well as different versions.
I haven't typed it in my PR either. The first build failed because I added type to parameter of
https://github.com/Codeception/module-webdriver/runs/4633062856?check_suite_focus=true so I removed it. I guess that PHP does not complain about return type that is more specific than the interface. |
|
Good point about documentation, I will check if we can use |
|
There is no inheritance, but I will try to implement it. |
Naktibalda
force-pushed
the
type-fixes
branch
from
c7046b7 to
d158de7
Compare
| @@ -1676,7 +1685,7 @@ public function _loadSession($session) | |||
| * | |||
| * @api | |||
| */ | |||
| public function _backupSession() | |||
| public function _backupSession(): ?RemoteWebDriver | |||
There was a problem hiding this comment.
@Naktibalda I just noticed that this method returns : array in Codeception 5
https://github.com/Codeception/Codeception/blob/a15b58781cbed2b3d5f21fd148cef474ee51101e/src/Codeception/Lib/Interfaces/MultiSession.php#L11
Should I remove the typing there?
| @@ -3108,36 +3132,36 @@ protected function matchFirstOrFail($page, $selector): WebDriverElement | |||
| * $I->pressKey('#name', array('ctrl', 'a'), \Facebook\WebDriver\WebDriverKeys::DELETE); //=>'' | |||
| * ``` | |||
| * | |||
| * @param $char string|array Can be char or array with modifier. You can provide several chars. | |||
| * @param string|array|WebDriverBy $element | |||
| * @param array<string|string[]>$chars Can be char or array with modifier. You can provide several chars. | |||
There was a problem hiding this comment.
@Naktibalda According to this typehint, $chars always has to be an array; so the given example $I->pressKey('#page', 'a'); would be invalid. Where did you take this from?
I fixed many signatures that were stricter than necessary.
Closes #70