Skip to content

Conversation

@ChristophWurst
Copy link
Contributor

And update the property constructor to better reflect that the
$parameters are an array of values, not instances of the parameter
class.

I suppose this can be seen as part of sabre-io/dav#1368.

Context: I converted some Nextcloud code into strict code and the section with properties/parameters was not understood by PHPStorm until I made these changes.

And update the property constructor to better reflect that the
$parameters are an array of values, not instances of the parameter
class.

Signed-off-by: Christoph Wurst <[email protected]>
ChristophWurst added a commit to nextcloud/server that referenced this pull request Jan 3, 2022
* List of parameters.
*
* @var array
* @var Parameter[]
Copy link
Member

Choose a reason for hiding this comment

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

I think we should go one step further:

Suggested change
* @var Parameter[]
* @var array<string, Parameter>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that as well, yet I'm not sure how "official" that phpdoc syntax is.

Copy link
Member

Choose a reason for hiding this comment

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

its kind of proprietary syntax which is used in psalm, phpstan and most static analysis tooling.

we could also use a more tool targetting syntax, so we don't kill the "offical form":

Suggested change
* @var Parameter[]
* @phpstan-var array<string, Parameter>
* @var Parameter[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you prefer this then I can add it

@ChristophWurst ChristophWurst deleted the refactor/type-property-parameters branch June 1, 2022 09:31
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.

2 participants