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

Mage_Catalog - DOC block update #783

Merged
merged 5 commits into from
May 28, 2020
Merged

Mage_Catalog - DOC block update #783

merged 5 commits into from
May 28, 2020

Conversation

sreichel
Copy link
Contributor

  • doc blocks added/fixed
  • PSR2 fixes (whitespaces, linebreaks, ...)

- doc blocks added/fixed
- PSR2 fixes (whitespaces, linebreaks, ...)
@sreichel sreichel added the Cleanup: DOC blocks Related to DOC block updates and fixes. label Jun 23, 2019
@colinmollenhour
Copy link
Member

I think PHPStorm 2020.1 has gotten a little smarter, it now recognizes the auto-completion better in @sreichel's example. It recognizes (new B)->foo() as returning A|B. However, using @return $this everywhere it says (new B)->foo() only returns B which I believe is more accurate (although not significantly so).

However, using @sreichel's example, PHPStorm 2020.1 flags a warning on C:

image

Using only @return $this there are no errors.

In my opinion we should always use @return $this when a method effectively does return $this; and not use @inheritDoc since the IDE's just are not as smart with handling it.

@tmotyl
Copy link
Contributor

tmotyl commented Apr 29, 2020

I agree with @colinmollenhour, especially as return $this is understandable for a human, whithout the need to dig deeper, while inheridoc is not.

@sreichel
Copy link
Contributor Author

However, using @sreichel's example, PHPStorm 2020.1 flags a warning on C:

Yes, it does. Just the tooltip is smarter.

In my opinion we should always use @return $this when a method effectively does return $this; and not use @inheritDoc since the IDE's just are not as smart with handling it.

Absolutely agree, but in this case its not return $this, it is parent::some() ....

But parent::_prepareLayout() just returns $this so @return $this is still correct - the return value is an instance of Mage_Catalog_Block_Layer_View.


I have used @inheritDoc only if return value calls parrent::x(), for return $this; i've used @return $this!

PLEASE leave it as it is for now.

colinmollenhour
colinmollenhour previously approved these changes May 5, 2020
@tmotyl
Copy link
Contributor

tmotyl commented May 6, 2020

I've resolved the conflict

@tmotyl
Copy link
Contributor

tmotyl commented May 15, 2020

conflict resolved once again

@tmotyl
Copy link
Contributor

tmotyl commented May 16, 2020

please revote

kkrieger85
kkrieger85 previously approved these changes May 19, 2020
Copy link
Contributor

@kkrieger85 kkrieger85 left a comment

Choose a reason for hiding this comment

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

LGFM

@@ -46,7 +46,7 @@ protected function _construct()
/**
* Retrieve resource instance wrapper
*
* @return Mage_Catalog_Model_Resource_Eav_Mysql4_Product_Action
* @inheritDoc
Copy link
Member

Choose a reason for hiding this comment

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

This is a step backward for type inference. @inheritDoc will mean that all methods added by Mage_Catalog_Model_Resource_Eav_Mysql4_Product_Action will no longer be detected by the IDE because the parent method's docs say the return is Mage_Core_Model_Mysql4_Abstract.

Copy link
Contributor

Choose a reason for hiding this comment

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

right. So what do you suggest?
do

     * @return Mage_Catalog_Model_Resource_Product_Action

?
Probably other places need to be rechecked

Copy link
Contributor Author

@sreichel sreichel May 22, 2020

Choose a reason for hiding this comment

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

I'd leave it as-is ...

  • Mage_Catalog_Model_Resource_Eav_Mysql4_Product_Action is an empty deprecated class
  • return parent::_getResource(); uses return type from parent DOCs ... so it seems correct
  • @return Mage_Core_Model_Mysql4_Abstract should be changed to resource (later)

Problem is Mage_Core_Model_Abstract::_getResource() ...

return Mage::getResourceSingleton($this->_resourceName);

... neither IDE nor phpstan can detect correct child-model of Mage_Core_Model_Mysql4_Abstract.

@sreichel sreichel added the hold label May 22, 2020
Co-authored-by: Colin Mollenhour <colin@mollenhour.com>
@sreichel sreichel removed the hold label May 22, 2020
@sreichel sreichel mentioned this pull request May 23, 2020
@tmotyl tmotyl merged commit 0a24d13 into OpenMage:1.9.4.x May 28, 2020
@sreichel sreichel deleted the cleanup/catalog branch May 29, 2020 02:01
@sreichel sreichel added the Component: Catalog Relates to Mage_Catalog label Jun 5, 2020
@sreichel sreichel added this to the Release 19.4.4 milestone Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup: DOC blocks Related to DOC block updates and fixes. Component: Catalog Relates to Mage_Catalog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants