Added type annotation to all methods#203
Added type annotation to all methods#203tolik518 wants to merge 10 commits intogitonomy:1.3from tolik518:1.3_more_annotations
Conversation
|
The PR is now ready for review @lyrixx |
lyrixx
left a comment
There was a problem hiding this comment.
Thanks for the PR. But I think this is not what we should do. See the first comment.
| * @param string $command | ||
| * @param array $args | ||
| * @param array $options | ||
| * | ||
| * @return Process |
There was a problem hiding this comment.
I think this is not the way to go. Instead we should add really PHP type.
It's easily doable for argument since PHP allow it in a BC manner.
But the return type will be a huge BC break!
There was a problem hiding this comment.
@lyrixx Could you please elaborate on what you mean by BC?
You mean instead of
* @param string $command
* @param array $args
* @param array $options
*
* @return Process
private static function getProcess($command, array $args = [], array $options = [])
one should write
* @param string $command
* @param array $args
* @param array $options
private static function getProcess($command, array $args = [], array $options = []): Process
I'm all for it but I thought this wouldn't work with PHP 5.6. If it does work - it won't work with mixed types at least.
If this is ok, could I go one step further and add PHP typing for all the arguments as well to ditch the DocBlocks entirely?
There was a problem hiding this comment.
BC mean Backward Compatible. see https://symfony.com/bc ; We should use the same policy.
The patch would be:
/**
* This internal method is used to create a process object.
*
* @return Process
*/
private static function getProcess(string $command, array $args = [], array $options = [])There was a problem hiding this comment.
Oh I see, I misunderstood you completely the first time.
So if I understood you correctly this time then I should add typed arguments instead of DocBlocks, but leave the Return in the DocBlocks?
I haven't worked much where BC was needed, so its better I'll ask you.
Is this case also valid when writing instead of this
/**
* @param Commit | string $hash
*
* @return Tag[] An array of Tag objects
*/
public function resolveTags($hash)this
/**
* @return Tag[] An array of Tag objects
*/
public function resolveTags(Commit | string $hash)There was a problem hiding this comment.
Unfortunately, this project is still compatible with PHP 5.6 😨 ! So wee cannot write this code (union). But with PHP 8+, you can 👍🏼
I'll bump this and I'll do a new release (next week I guess)
There was a problem hiding this comment.
Perfect. Should I change everything that's not a union-type to php types instead of DocBlocks though like you required?
There was a problem hiding this comment.
I just figured out that primitive type-hints weren't supported until PHP 7.0.
So all I can do is add native type-hints only for arrays and objects.
There was a problem hiding this comment.
can you wait a bit? I'll bump some things...
* Bump minimal PHP version to 8.0 * Fixed ci when pushing on origin repo
I'm currently working on adding Docblocks with type annotations and throws to all methods to get full type-hinting through the IDE.