-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add default value for new parameter #53
Conversation
src/Poser.php
Outdated
@@ -34,7 +34,7 @@ public function __construct($renders) | |||
* @param $style | |||
* @param $format | |||
*/ | |||
public function generate(string $subject, string $status, string $color, string $style, string $format): Image | |||
public function generate(string $subject, string $status, string $color, string $style, string $format = 'svg'): Image |
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.
Maybe use null
value here and assign it on svg
if null inside of the methods? And it will be better to use Badge::DEFAULT_FORMAT
here.
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.
what's the point of passing null
and assigning value later?
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 anybody extends Poser class, then default value may be different then.
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.
Can't be different even now?
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.
I was wrong, you may overwrite default value in extended class. Just saw this way of defining default values very frequently and thought it's protecting vs this case.
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.
in my opinion, it's okay to set the default here, but I'd use the constant Badge::DEFAULT_FORMAT
src/Poser.php
Outdated
@@ -34,7 +34,7 @@ public function __construct($renders) | |||
* @param $style | |||
* @param $format | |||
*/ | |||
public function generate(string $subject, string $status, string $color, string $style, string $format): Image | |||
public function generate(string $subject, string $status, string $color, string $style, string $format = 'svg'): Image |
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.
in my opinion, it's okay to set the default here, but I'd use the constant Badge::DEFAULT_FORMAT
@garak please update the changelog! Thanks! |
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.
LGTM
For BC