-
Notifications
You must be signed in to change notification settings - Fork 736
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 CS bis #1706
Fix CS bis #1706
Conversation
5817b5b
to
bcdf6f7
Compare
Not yet ready. I will add a new comment when it’s ready. |
@@ -35,7 +35,7 @@ public function setBucketsPath(string $bucketsPath) | |||
* | |||
* @return $this |
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.
The return $this
should be removed too :)
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.
Lets separate PR's which are fully automated and manual changes ;-)
@@ -32,7 +32,7 @@ class HttpAdapter extends AbstractTransport | |||
* | |||
* @param Connection $connection | |||
*/ | |||
public function __construct(Connection $connection = null, HttpAdapterInterface $httpAdapter) | |||
public function __construct(?Connection $connection = null, HttpAdapterInterface $httpAdapter) |
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.
There is a glitch here, optional argument before a non optional argument.
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.
If we don't make it optional, would things break?
31bbeda
to
d0213d4
Compare
@deguif Any chance you could update the PR description to contain a bit more details on the change? I'm aware the comment is linked but if someone comes back to this and asking why this all changed, would be nice to have it in a single place summarised. |
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.
Change LGTM. We can still follow up with more minor changes.
@thePanz WDYT?
@ruflin Ok to merge! I'd squash the changes, just to keep the git history clean. wdyt? |
This PR add two new rules for coding style:
nullable_type_declaration_for_default_null_value
to enforce?
declaration on nullable arguments. See ongoing discussion on Deprecate implicit nullable parameters php/php-src#3535no_alias_functions
to remove uses ofsizeof
, ... PHP aliases functions.