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

Fix @type to @var #186

Merged
merged 4 commits into from Apr 16, 2019
Merged

Fix @type to @var #186

merged 4 commits into from Apr 16, 2019

Conversation

bizurkur
Copy link
Contributor

  • Replaced @type with @var
    • @type is not a valid phpdoc tag.
    • @var helps static analysis tools identify what properties should contain.
  • Fixed phpstan errors
  • Updated @deprecated to @api based on this discussion

bizurkur added 4 commits April 15, 2019 18:15
- vfsStreamFile allows any FileContent to be used as the file,
however the interface is missing these methods so not every
implementation of FileContent would be acceptable.
- All existing implementations contain these methods.
@bizurkur bizurkur added this to the 2.0.0 milestone Apr 16, 2019
Copy link

@rokclimb15 rokclimb15 left a comment

Choose a reason for hiding this comment

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

Looks good, save one question about the mixed array PHPDoc.

*/
protected $exclusiveLock;
/**
* Resources ids which currently holds shared lock to this file
*
* @type bool[string]
* @var array<string, bool>

Choose a reason for hiding this comment

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

Is this valid PHPDoc? I have never seen this style used before with any PHP documentation parser/generator.

I would think bool[]|string[]. If you wanted more accuracy, (bool|string)[]. This form is from the PSR-5 draft and isn't widely supported yet though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually a bool[] indexed by string. The doc format is PHP Generics, which is still just an RFC but is what phpstan uses to add more detail to known data structures. It's essentially a <key, value> comment, in this case. Generics can be used for a lot more than that. There's big projects that have some usage of it, but not a lot.

Documenting it using the generics syntax will help static analysis tools ensure data quality, as it should trigger an error if someone ever tries to use a non-string as an index in that array.

Alternatively, it could be @var bool[] Indexed by string but it'll be a little less helpful for static analysis.

Choose a reason for hiding this comment

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

Ah I see! I knew about the Generics syntax, but didn't realize you could apply it to array.

Copy link
Contributor

Choose a reason for hiding this comment

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

And yes this is a valid phpdoc, we introduced that I think about 4 years ago ;-)

Copy link
Contributor

@jaapio jaapio left a comment

Choose a reason for hiding this comment

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

I had my doubts about the @deprecated changes. But since you discussed it I think it will be fine.

@jaapio jaapio merged commit 96035bb into bovigo:master Apr 16, 2019
@bizurkur bizurkur deleted the fix-doc-blocks branch April 17, 2019 00:05
@bizurkur bizurkur mentioned this pull request Apr 17, 2019
@mikey179
Copy link
Member

Uhm... a little late to the party, but the methods which switched from @deprecated to @api should not be part of the public API. Rather, they should be marked as @internal. They are required for vfsStream to function properly and for the unit tests, but a user of the library doesn't need them.

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.

None yet

4 participants