-
-
Notifications
You must be signed in to change notification settings - Fork 75
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.
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
Fix few problems reported by PHPStan #192
Conversation
❌ |
Hey @szepeviktor, Thank you for taking the time to prepare a PR! I can see there are areas that need improving. I've replied to your comments and changes. Could you have a look and check the PHPStan issues? Do you think anything else should go into this? Cheers, |
Yes. Your work :) |
public function __construct(?array $config = []) | ||
/** | ||
* @param PHPScraperConfig $config | ||
*/ | ||
public function __construct(array $config = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed null as the base value of the configuration array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW arrays are not for carrying nonhomogeneous elements.
Arrays are for lists: keyless elements of the same type.
Configuration should be a self-contained object.
$config->followRedirects();
$config->dontFollowRedirects();
$config->getHttpClientConfiguration(); // returns that array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, introducing an object to contain the config would be neat
@@ -39,9 +45,9 @@ public function __construct(?array $config = []) | |||
/** | |||
* Sets the config, generates the required Clients and updates the core with the new clients. | |||
* | |||
* @param ?array $config = [] | |||
* @param PHPScraperConfig $config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could be as follows if your IDE complains.
* @param array<string, mixed> $config
* @phpstan-param PHPScraperConfig $config
The next step is to remove |
@@ -11,23 +11,29 @@ | |||
use Symfony\Component\BrowserKit\HttpBrowser; | |||
use Symfony\Component\HttpClient\HttpClient as SymfonyHttpClient; | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's a great solution to telling PHPStan better what is in the variables. Thanks!
public function __construct(?array $config = []) | ||
/** | ||
* @param PHPScraperConfig $config | ||
*/ | ||
public function __construct(array $config = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be fine
@spekulatius If you like robust code you may continue fixing the rest of the problems.
(otherwise just close it - this is an inspiration)