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

Minor doc fixes #476

Closed
wants to merge 1 commit into from
Closed

Conversation

theofidry
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR not required

- Fix a PHPDoc return type comment
- Add a missing return statement
- Add a TODO comment
@@ -60,7 +60,7 @@ public function getType()
*
* @param Type $type
*
* @return self
* @return PropertyMetadata
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using self here is misleading as it implies you are returning the same object, which is not the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

self is correct. You have $this in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

self means an object of the same class, not the same instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me in this case (return value of the phpdoc), self and this would mean the exact same thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But as you said it's still a draft, it might change. And as it's still a draft, and not very widespread, I'm more concern on how our library consumer may interpret it. Even if both are technically valid, I would rather pick the more intuitive one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it's here too FWIW: https://www.phpdoc.org/docs/latest/references/phpdoc/types.html

The PSR standard is still a draft, but as far as phpDocumentor (the library) is concerned, that's how it should be. And I don't think anyone would want to deviate from that without good reason (since I'd think it's also how IDEs etc. have implemented it)...

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'd think it's also how IDEs etc.

Well, besides finding that unintuitive, IDE do not recognise it either (at least PhpStorm 2016.1)

But if you're finding it being more of an hassle that' fair, I just want to raise that point :)

Copy link
Member

Choose a reason for hiding this comment

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

👎 to change this. self is perfectly valid according to the current and future PHPDoc standard. It also eases the refactoring and improves the readability.

@theofidry
Copy link
Contributor Author

Ok, so let's stick with self then

@theofidry theofidry closed this Mar 19, 2016
@theofidry theofidry deleted the minor-changes branch March 19, 2016 18:21
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

3 participants