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

Code reviews welcome #973

Closed
sreichel opened this issue May 13, 2020 · 9 comments
Closed

Code reviews welcome #973

sreichel opened this issue May 13, 2020 · 9 comments
Labels
Cleanup: Code style Related to simple CS fixes. Cleanup: DOC blocks Related to DOC block updates and fixes.

Comments

@sreichel
Copy link
Contributor

sreichel commented May 13, 2020

Hey

i'm contributing here since years, now i ask for your help. Please do some reviews,

I spent weeks to add doc blocks and annotions to make work easier. These PRs are nearly one year old and still lack of reviews. They are not perfect and can/will be improved ....

I'd like to help with "serious" problems, but I also want to have the code style issues been fixed ....

Thanks!

@sreichel sreichel added help wanted Cleanup: DOC blocks Related to DOC block updates and fixes. Cleanup: Code style Related to simple CS fixes. labels May 13, 2020
@sreichel
Copy link
Contributor Author

sreichel commented May 18, 2020

Just one review(s) left, thanks @tmotyl.

Review size ... (progress here #725, moved to #416)

nothing left ... (so far)

@sreichel sreichel pinned this issue May 20, 2020
@sreichel sreichel unpinned this issue Jun 2, 2020
@sreichel
Copy link
Contributor Author

sreichel commented Jun 5, 2020

Woooha :))

Thanks @colinmollenhour for latest reviews .... with Mage_Eav there is only one "bigger" PR left. Nice progress 👍

@colinmollenhour
Copy link
Member

I'm doing what I can, those things are brutal. :) Nice work though, and thanks also to @tmotyl for being first to review them!

@sreichel
Copy link
Contributor Author

Only 17 small one left ... nice 👍

@sreichel
Copy link
Contributor Author

sreichel commented Jul 6, 2020

Thanks @colinmollenhour for merging last ones, thanks for reviews .... I already work on Adminhtml` and it su* .... it is painful.

@tmotyl
Copy link
Contributor

tmotyl commented Jul 7, 2020

@sreichel would it be possible to add some static analysis with whitelist for the modules which are fixed now, or is it too early?

@sreichel
Copy link
Contributor Author

sreichel commented Jul 8, 2020

@tmotyl yes i can do, but ...

  • even for level 0 or 1 you have to exclude sniffs.(e.g missing parent constructor or properbly undefined vars in set in try/catch-blocks)
  • exclude some files completly where it is not possible to resolve return values (maybe /ignoreMethod option is added in later phpstan release)
  • still use far outdated phpstan 0.8.x ... magento1-extension has to be updated for phpstan 0.12.x. Someone? (there is already closed a PR for v0.12.x)

@tmotyl
Copy link
Contributor

tmotyl commented Jul 8, 2020

Can we use fork https://github.com/vianetz/phpstan-magento1/ (which sounds to be compatible) directly or does it require additional changes (then I would go for a fork in openmage)

@sreichel
Copy link
Contributor Author

sreichel commented Jul 9, 2020

I'll try it out over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup: Code style Related to simple CS fixes. Cleanup: DOC blocks Related to DOC block updates and fixes.
Projects
None yet
Development

No branches or pull requests

3 participants