Code reviews welcome - Githubissues

OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.

https://www.openmage.org

Open Software License 3.0

869 stars 436 forks source link

Code reviews welcome #973

Closed sreichel closed 4 years ago

sreichel commented 4 years ago

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 commented 4 years ago

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

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

nothing left ... (so far)

sreichel commented 4 years ago

Woooha :))

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

colinmollenhour commented 4 years ago

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 commented 4 years ago

Only 17 small one left ... nice :+1:

sreichel commented 4 years ago

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

tmotyl commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

I'll try it out over the weekend.