Skip to content

Conversation

@kenjis
Copy link
Member

@kenjis kenjis commented Oct 25, 2022

Description
Explain what you have changed, and why.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

Co-authored-by: MGatner <[email protected]>
@MGatner
Copy link
Member

MGatner commented Oct 27, 2022

@kenjis it seems like you've made a move to keep PHPDoc tags strictly to the PHPDoc officially supported annotations. I would like us to step back and find consensus on our approach, because this is not something we have held to before.

I could see this going either way. If we start using vendor tags, do other vendors miss out on the enhanced types? Or, if we only use the vanilla tags: does PHPDoc or certain vendors fail to parse correctly?

If anyone has answers or best practices please share. I will plan to do some research on the subject as well.

@codeigniter4/core-team

@paulbalandan
Copy link
Member

In my repos, I always strive to use the generally available PHPDoc tags. However, if there's a chance to enhance the type further with specific tags, for example with PHPStan, I will add the phpstan-specific tags.

@kenjis kenjis force-pushed the fix-phpdoc-Factories branch from 3b56fe0 to 34f0867 Compare October 27, 2022 12:48
@MGatner
Copy link
Member

MGatner commented Oct 28, 2022

@paulbalandan Does that mean you never use <> notation on a vanilla annotation?

@MGatner
Copy link
Member

MGatner commented Oct 28, 2022

I'm seeing three levels here:

  1. values explicitly defined by PHPDoc
  2. values generally supported by major IDEs and static analyzers
  3. value explicitly created by one vendor

The problem seems to be that 1. is clearly defined but 2. and 3. are not. Probably some more research (I haven't done it yet) could help clarify but it is probably a bit of a moving target.

We should define explicitly what we want to support product-wise so once we have an idea of 2 versus 3 we can make a policy (unless of course we only want 1 for vanilla tags and everything else is vendored).

@paulbalandan
Copy link
Member

@paulbalandan Does that mean you never use <> notation on a vanilla annotation?

On regular tags like @var I interchangeably use array<int, string> and string[] because intelephense understands both. But I use array shapes array{0: string} for phpstan specific tags.

@MGatner
Copy link
Member

MGatner commented Oct 28, 2022

Intelephense and PHPStorm both support array shapes (I use them a lot at work), and I think VSCode does as well (for sure with Intelephense).

@paulbalandan
Copy link
Member

I'm using intelephense for vscode and it cannot recognize array shapes yet.

@MGatner
Copy link
Member

MGatner commented Oct 29, 2022

Now I'm going to have to go test... I'm using Intelephense in BBEdit and I swear it recognizes array shapes but maybe not??

FYI this has been on my list to get set up and try as an alternative to Intelephense: https://github.com/phpactor/phpactor It basically lets you use PHPStan as an LSP!

@kenjis
Copy link
Member Author

kenjis commented Oct 29, 2022

VS Code deep-assoc-completion seems to support array shapes.
https://plugins.jetbrains.com/plugin/9927-deep-assoc-completion

@kenjis
Copy link
Member Author

kenjis commented Oct 31, 2022

@MGatner Can I merge this?

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Please do.

@kenjis kenjis merged commit a8a99dc into codeigniter4:develop Oct 31, 2022
@kenjis kenjis deleted the fix-phpdoc-Factories branch October 31, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants