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
feat: update ECS config and apply standards #2893
Conversation
I see we are no longer prefixing the docblock return types with the full namespace. Could this have a negative impact in the symbol discovery of IDE's? |
I've just tested the doc generation in combination of your branch and the docs still seem to compile the correct paths :) |
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 suggested some docblock updates while at it and there is one return type issue that we might already discussed in a ticket. Just checking. Other than that, looks good to me.
@@ -67,7 +66,7 @@ trait AccessesPostsLazily | |||
* // No additional overhead here. | |||
* } | |||
* ``` | |||
* @return \Timber\PostCollectionInterface The realized PostQuery. | |||
* @return PostCollectionInterface The realized PostQuery. |
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.
This throws a static analysis error. Is this something we must take care of? Not sure if this was something that we already discussed in an issue.
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 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.
Haven't figured it out yet but this shouldn't prevent us from merging the PR. We don't require PHPStan to pass (yet). :)
3d86c7b
to
917b51c
Compare
Co-authored-by: Erik van der Bas <erik@basedonline.nl>
917b51c
to
315dc67
Compare
@gchtr , can you take a final look at this? |
It’s not a problem, because the reflections still work and generate fully qualified names, apparently. It only doesn’t work for hook documentation. There, the types have to be generated from the DocBlock. As you can see in the following screenshot, we could have These hook DocBlocks are parsed by looping over the tokens in https://github.com/timber/teak/blob/a5af1504cadb8e5d2277069165caf948d1d0c012/lib/Compiler/HookReference.php#L62-L70. I guess I could somehow also derive the types from the variables that are present before a hook appears. I’ll have to check this out. I created an issue for this in timber/teak#3. For now, I think it’s a tradeoff we can live with, because it only affects some few filters in https://timber.github.io/docs/v2/hooks/filters/. |
Thanks @gchtr for the details about doc generation. There should be a way to grab the FQCN, let me know if I can help. |
Bump ECS config.
Note it changed quite a lot a files and we might want to add this commit in the .git-blame-ignore-revs file.
It also removed to full namespace from the docblocks, I don't if this cause issues when generating API reference? @gchtr