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

PHPMD: added basic config #2771

Merged
merged 11 commits into from
Dec 28, 2022
Merged

PHPMD: added basic config #2771

merged 11 commits into from
Dec 28, 2022

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Dec 2, 2022

Description (*)

Last tool to add ... PHPMD

  • added composer (dev) dependency (already there)
  • added default config file
  • suppress error message for @ operator

Todo (new PRs)

Test

vendor/bin/phpmd app/code/core/Mage/ text .phpmd.dist.xml

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@sreichel sreichel marked this pull request as draft December 2, 2022 02:04
@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Backup Relates to Mage_Backup Component: Catalog Relates to Mage_Catalog Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Eav Relates to Mage_Eav Component: Install Relates to Mage_Install Component: lib/Varien Relates to lib/Varien Component: lib/Mage Relates to lib/Mage Component: lib/* Relates to lib/* Component: Media Relates to Mage_Media Component: PayPal Relates to Mage_Paypal Component: Sales Relates to Mage_Sales labels Dec 2, 2022
@github-actions github-actions bot added the Component: lib/Magento Relates to lib/Magento label Dec 4, 2022
@sreichel sreichel changed the title Add PHPMD to workflow PHPMD: added basic config Dec 5, 2022
@sreichel sreichel marked this pull request as ready for review December 5, 2022 23:42
fballiano
fballiano previously approved these changes Dec 17, 2022
@fballiano
Copy link
Contributor

do we need it? does things that phpstan does not? because now we're adding a lot of "suppress warning" which... I don't like.

@sreichel
Copy link
Contributor Author

do we need it?

Do we need codesyle checks or phpstan? Imho phpmd is a great tool to improve code.

does things that phpstan does not?

Yes. Its completly different. phpstan checks for working code, phpmd for clean (not messed up) code.

because now we're adding a lot of "suppress warning" which... I don't like.

Add "suppress warning" shows that the "problematic" code is intentional - or needs to be reviewed.

I'd like to enable checks step by step ... regardless of adding some lines to phpdocs.

@fballiano
Copy link
Contributor

ok I see, but still, we're not going to add "suppresswarning" to every method just to remove it later one by one right?

@sreichel
Copy link
Contributor Author

Nope. It should only be added for code that cant be changed.

@fballiano fballiano merged commit 19e34ed into OpenMage:1.9.4.x Dec 28, 2022
@sreichel sreichel deleted the phpmd-1-cleancode branch December 28, 2022 15:36
fballiano pushed a commit to fballiano/openmage that referenced this pull request Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Backup Relates to Mage_Backup Component: Catalog Relates to Mage_Catalog Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Eav Relates to Mage_Eav Component: Install Relates to Mage_Install Component: lib/Mage Relates to lib/Mage Component: lib/Magento Relates to lib/Magento Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Media Relates to Mage_Media Component: PayPal Relates to Mage_Paypal Component: Sales Relates to Mage_Sales
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants