Skip to content
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

NFC - Introduce array shape annotations in docblocks #21675

Merged
merged 1 commit into from
Oct 3, 2021

Conversation

colemanw
Copy link
Member

Overview

First docblock to use the new array shapes annotation 🎉

First docblock to use the new array shapes annotation
@civibot
Copy link

civibot bot commented Sep 30, 2021

(Standard links)

@civibot civibot bot added the master label Sep 30, 2021
@@ -259,7 +260,7 @@ protected function formatCustomParams(&$params, $entityId) {
*
* @param string $fieldExpr
* Field identifier with possible suffix, e.g. MyCustomGroup.MyField1:label
* @return array|NULL
* @return array{id: int, name: string, entity: string, suffix: string, html_type: string, data_type: string}|NULL
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes PhpStorm actually understand what the returned array contains!

Copy link
Contributor

Choose a reason for hiding this comment

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

Vim doesn't care but this confuses vscode a little bit. It now thinks the return var is \Civi\Api4\Generic\Traits\array. It doesn't personally bother me too much - not sure who else uses vscode.

Copy link
Member

Choose a reason for hiding this comment

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

I do think a few people use VSCode.

FWIW, there's some discussion on bmewburn/vscode-intelephense#249 about adding support for "array shapes". They point out that a few tools (psalm, phpstan, phan, phpstorm) now support the notation. There's a link to https://marketplace.visualstudio.com/items?itemName=klesun.deep-assoc-completion-vscode

FWIW, PHPStorm 2019.3 seems happy with the current snippet.

Verbalizing, some options:

  1. Press forward with array-shape. The world will adjust!
  2. Hold off on array-shape
  3. Use for some clever in-between for a transitional period. (Maybe array|array{id: int...}|NULL? It seems OK on PHPStorm 2019.3?)
  4. Leave it in ambiguity

FWIW, I'm OK with 1 or 2 or 3 - but 👎 on 4.

Copy link
Contributor

@demeritcowboy demeritcowboy Oct 1, 2021

Choose a reason for hiding this comment

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

Based on that issue it sounds like something will happen with intelliphense eventually, even if it's just the comment earlier in the thread "Even If you're not planning to add feature this syntax should be at least ignored."

By the way I did try to see what happens if the function also has a php type hint : array. It then thinks the return value is exit. (Yep.) And that doesn't work anyway here because it can be null. If I remove the null from the phpdoc and use the typehint : array, then it thinks the return value is never. (Yep.)

There is another php plugin for vscode but it causes the whole computer to grind to a halt if you try to open a drupal 8 folder, so it probably doesn't handle this either.

Your option 3 sort of works. It thinks it's array|\Civi\Api4\Generic\Traits\array{id:. But either way none of this changes what you can do with it in terms of code-completion for vscode because it's an array anyway not an object. So for arrays it's just a visual hint in vscode, so also combined with that github issue me personally I'm fine with option 1. It doesn't prevent you typing $x[0] or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like we're all fine with option 1.

@colemanw colemanw merged commit 3f161ba into civicrm:master Oct 3, 2021
@colemanw colemanw deleted the commentCleanup branch October 3, 2021 17:50
@demeritcowboy demeritcowboy changed the title NFC - Update code comments NFC - Introduce array shape annotations in docblocks Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants