Add new tags #373
Add new tags #373
Conversation
|
I also thinks I should open a pr for type resolver repository beacuse of this classes not handled: |
|
Right now we do not focus on the prefixed tags used by psalm and phpstan. Integrations do have the option to add new tags and register a factory twice to handle tags. I don't thing we should support this out of the box. The tags are mend to use by these tools. And are not general purpose tags. Regarding the extra types, it would be nice to support them. But I'm wondering if we should add them to the type resolver and how. As users of the type resolver will assume that they get clear types and apart from the objectshape they are all fuzzy. Maybe we should introduce them as |
|
Well I add them to About prefixed tags, I was thinking at least add some important of them. (lots of them will be generics and no need to new classes) |
Ok I will fix them tommorow |
|
I guess I'm done except I did not found |
There was a problem hiding this comment.
I'm sorry, I must have overlooked the things I commented on in this review. It's a bit hard to review this while I'm just on a mobile phone as I'm on holiday right now. But I didn't want to let you wait for another 3 weeks.
It's ok if you can answer my questions. We can always fix some things after this has been merged. I will not do a release before I'm able to do some tests myself, just to be sure we do not overlook things.
| $description = $tagValue->description; | ||
| } | ||
|
|
||
| $class = $node->name === '@implements' ? Implements_::class : TemplateImplements::class; |
There was a problem hiding this comment.
Why would you combine these tags in 1 factory? I think it's wise to keep them separate. Maybe with a base class. A factory should always return a single type in my opinion. Otherwise this is an abstract factory
There was a problem hiding this comment.
Well I just think avoid duplicate codes.
it will be easy for me to seprate them
| /** | ||
| * Reflection class for a {@}template-extends tag in a Docblock. | ||
| */ | ||
| final class TemplateExtends extends Extends_ |
There was a problem hiding this comment.
Is Template extends a decorator on extends? And could you replace template-extends with extends? Because that's what it means when you extend a tag this way. I'm not sure that's what you intended to do.
In this project we chose often to duplicate code because tags are not the same and cannot be interchanged. If we want to share code between tags we chose to create a base class.
I'm not aware of what template-extends does. And I couldn't find it in the phpstan documentation. So it could be that you are fully correct here.
| { | ||
| $this->name = 'template'; | ||
| $this->templateName = $templateName; | ||
| $this->bound = $bound; |
There was a problem hiding this comment.
What is the bound property? I don't know what it means in this context. Can we clarify that by renaming the property or add some docblock that explains the value?
There was a problem hiding this comment.
I just copied property name from phpstan but as far I know:
template T of int
int is bound
|
I fix these things but why for god sake there is not a script in composer.json for phpcs-fixer |
|
We are using php-code-sniffer, there is a target in the make file named "code-style" to run it. For historical reasons this project didn't have dev dependencies. Especially when phpunit had a direct dependency this was problematic. We could never build on a new major version without a version issue on phpunit. |
|
When I'm back I will merge this and add some more tests to cover this new behavior. Thanks for your efforts |
This is a beta pr for checking my code.
I will add template tag for sure.
But what about psalm, phan, phpstan tags? Should I handle them too?