✨ method parameter default value rendering #374
Conversation
|
@jaapio don't hesitate i'm available if this PR needs something 😉 |
|
Thanks for your contribution. It looks like phpstan has spot some issues that make the test fail. Could you please have a look at that? |
|
Should be fixed as verified with phpstan playground |
src/DocBlock/Tags/Method.php
Outdated
| $parameterDefaultValueStr = null; | ||
| if ($parameter->getDefaultValue() !== null) { | ||
| $parameterDefaultValueStr = $parameter->getDefaultValue(); | ||
| settype($parameterDefaultValueStr, (string)$parameter->getType()); |
There was a problem hiding this comment.
This doesn't seem to be correct to me. Default values can be complex definitions.
It might be good to check with a number of testcases to ensure this works as expected.
Please do also add a testcase with a custom class.
There was a problem hiding this comment.
You mean isolate the test in a test class dedicated to this?
There was a problem hiding this comment.
Oh no i understood, give me some time 😄
|
You were right, it didn't cover enough cases, I did rewrote the concept, what do you think 😊 |
|
@jaapio did you have any time looking at it maybe ? 😄 |
There was a problem hiding this comment.
In general this looks very nice. I have a few requests to change the implementation that will help me to keep this working in the future and allow me to improve. Those requests might sound strange, but they are based on years of experience with this codebase. I try to keep things as closed as possible, to allow me to do major refactorings when needed without breaking changes.
Regarding these breaking changes I do have a few doubts. We are changing the return type of the getDefaultValue method. I do get that we need to change the type of the property which can be code as we stretch the accepted values. Maybe we should use the property directly in the __toString method as that will give us the information we need. But we can keep the return value of getDefaultValue a string?
| use function str_repeat; | ||
| use function strlen; | ||
|
|
||
| class MethodParameterFactory |
There was a problem hiding this comment.
Please mark this class as final and @internal as we should not make this part of the backward compatibility promise. Extending this class should be blocked as it is an internal only class. Maybe this should even be part of the Method tag? Unless there is a reason to extract this because you want to share this class?
There was a problem hiding this comment.
All good
For me, extracting this logic from the base code is a good idea for the maintanability
It could have been a trait but I did get inspired by the package way of working, could be wrong
| return ''; | ||
| } | ||
|
|
||
| protected function formatDouble(float $defaultValue): string |
There was a problem hiding this comment.
I do prefer private for all new methods as we do not want others to interfere with this code. This make it easier to refactor methods as private methods are internal only, and we can remove them at will.
|
I'm not quite sure what's going on here, if you have an idea, don't hesitate 😊 |
|
All good I figured it out |
| @@ -60,8 +69,17 @@ public function isVariadic(): bool | |||
| return $this->isVariadic; | |||
| } | |||
|
|
|||
| public function getDefaultValue(): ?string | |||
| public function getDefaultValue(): mixed | |||
There was a problem hiding this comment.
Can we please revert this as I did notice before, because I see this as a backward compatibility break. I think we should not do that right now. This will also solve the failing tests
| } | ||
|
|
||
| /** | ||
| * @param array<array, null, int, float, bool, string, object> $defaultValue |
There was a problem hiding this comment.
| * @param array<array, null, int, float, bool, string, object> $defaultValue | |
| * @param array<array|null|int|float|bool|string|object> $defaultValue |
|
Seems all good |
|
I added the fact that it returns "null" when no value was set as before |
|
Thanks again for your contribution! |
For now the default value for method parameter is not used but present, when rendering, default value is never used so it can lead to misleading behavior where your IDE tells you to fill a value but it has a default one so it's not needed
In this PR I added the ability for method parameter to implement this