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

[WIP] Cleanup: Round 1 - DOC block updates #725

Closed
50 of 54 tasks
sreichel opened this issue Jun 15, 2019 · 10 comments
Closed
50 of 54 tasks

[WIP] Cleanup: Round 1 - DOC block updates #725

sreichel opened this issue Jun 15, 2019 · 10 comments
Labels
Cleanup: DOC blocks Related to DOC block updates and fixes. duplicate

Comments

@sreichel
Copy link
Contributor

sreichel commented Jun 15, 2019

Just an overview for already opened or ready PRs ... I'll add further PRs as soon I think I#m done with round 1 :)

Round 1:

  • just doc blocks
  • PSR2 automatic fixes
  • all other changes (added missing properties/...) will be removed
  • there are some missing typehints for parameters ... pls don't worry, will be fixed in round 2 :)

Pull requests ...

  • Admin
  • Adminhtml - classname designpack
  • AdminNotification
  • Api
  • Api2
  • Bundle
  • Captcha
  • Catalog
  • CatalogIndex
  • CatalogInventory
  • CatalogRule
  • CatalogSearch
  • Checkout
  • Cms
  • ConfigurableSwatches
  • Contacts
  • Core
  • Cron
  • CurrencySymbol
  • Customer
  • Directory
  • Downloadable
  • Eav
  • GiftMessage
  • ImportExport
  • Index
  • Log
  • Media
  • Newsletter
  • Oauth
  • Page
  • PageCache
  • Payment
  • Persistent
  • Poll
  • ProductAlert
  • Rating
  • Reports
  • Review
  • Rss
  • Rule
  • Sales
  • SalesRule
  • Sendfriend
  • Shipping
  • Sitemap
  • Tag
  • Tax
  • Uploader (0)
  • Weee
  • Widget
  • Wishlist

Changes in lib

  • Varien
  • Zend

Exluded (at least in "round 1"):

  • Authorizenet
  • Backup
  • Centinel
  • Connect
  • Dataflow
  • GoogleAnalytics
  • GoogleBase (removed, see Removed Mage_GoogleBase #1002)
  • GoogleCheckout
  • Install
  • Paygate
  • Paypal
  • PaypalUk
  • Usa
  • XmlConnect
@sreichel sreichel pinned this issue Jun 15, 2019
@sreichel sreichel unpinned this issue Jun 15, 2019
@colinmollenhour
Copy link
Member

Nice work, @sreichel and thanks for tackling this big project! I'm fully on-board with all of the doc block-only updates I've seen so far. Thanks for splitting them up from the other updates so they can be reviewed separately.

Does anyone have any objection to deviating from the normal 2+ reviewers for the doc-block-only PRs as long as the Travis-CI checks passed (no PHP syntax errors)? The faster we get these merged the less likely we'll run into merge conflicts with future PRs.

@tmotyl
Copy link
Contributor

tmotyl commented Jun 17, 2019

go for it :)

@sreichel
Copy link
Contributor Author

sreichel commented Jun 17, 2019

Does anyone have any objection to deviating from the normal 2+ reviewers for the doc-block-only PRs as long as the Travis-CI checks passed (no PHP syntax errors)?

How about ... I'll request rewievs as soon i think the PR is ready? @colinmollenhour @Flyingmana @tmotyl ?

Most current PRs are small modules, but changelists are already long. Comming PRs include bigger ones (Sales, Catalog, ...) and have PSR2 errors fixed too (whitespaces, linebreak, indentations) so changelists get more longer ...

I could also split PRs into 2-3 commits (block, models, helpers, ....) if needed?

(just working on Adminhtml, Catalog and Sales .... expect ~50 PRs during next days :P)

The faster we get these merged the less likely we'll run into merge conflicts with future PRs.

👍 I'prefer this PRs get merged before all other code changes ...

@sreichel sreichel added the Cleanup: DOC blocks Related to DOC block updates and fixes. label Jun 17, 2019
@sreichel
Copy link
Contributor Author

I'm done! :)

Startet with far +25k warnings in PHPstorm, complaining about undefined methods, now its down to >8k.

Most unresolved issues are calls on Varien_Objects or in unedited modules like XmlConnect ...

EVERY method should have a doc block now. Typehints for parameter are nearly complete - possible that i've missed some. In this case please just leave a comment, w/o request changes. I'll collect it and fix it at the end with one PR.

@sreichel sreichel changed the title [WIP] DOC block updates [WIP] Cleanup: Round 1 DOC block updates Jun 23, 2019
@sreichel sreichel changed the title [WIP] Cleanup: Round 1 DOC block updates [WIP] Cleanup: Round 1 - DOC block updates Jun 23, 2019
@sreichel
Copy link
Contributor Author

@tmotyl i've just requested reviews from @Flyingmana and @colinmollenhour. Would like to help? :)

@rvelhote
Copy link
Contributor

rvelhote commented Jul 1, 2019

Thank you so much for the hard work!
Can you please let me know what tools/plugins/configs are you using for these docblock reviews with PHPStorm? I always have MessDetector + Code Sniffer when developing.

@sreichel
Copy link
Contributor Author

sreichel commented Jul 1, 2019

@rvelhote I've used PHPStorm + CS + xDebug and with more added doc blocks phpstan. Phpstan shows missing typehints, type-mismatches, ...

@sreichel
Copy link
Contributor Author

sreichel commented Aug 2, 2019

Mhhh. Bit disappointing ...

@Flyingmana
Copy link
Contributor

Sorry, properly reviewing all this changes is quite time consuming and we have a certain lack of reviewers. With the amount of changed lines this will likely take also most of next year, till we are through all of them.

@sreichel
Copy link
Contributor Author

See #416

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. duplicate
Projects
None yet
Development

No branches or pull requests

5 participants