phpmd icon indicating copy to clipboard operation
phpmd copied to clipboard

Add prefix removal for class names and variable names

Open JoshuaBehrens opened this issue 3 years ago • 3 comments

Type: feature / documentation update Issue: Adds prefix removal for variables and class names Breaking change: no Changed documentation pages:

  • https://phpmd.org/rules/naming.html

My issue is for class names that need to follow a pattern. E.g. doctrine/migrations expect migration classes to have a prefix. Following a functional pattern should not count to a meaningful name limitation. phpmd does not support this yet.

As we also have a rule for variable names I thought about this change being useful there as well. I think about Hungarian notation name schemas. I don't use them and I support statements that they are dated but it is a thing. In addition I can imagine similar situations for properties like patterns in the class name situation. Sooo I just added it as well.

Please check this points before submitting your PR.

  • [x] Add test to cover the changes you made on the code.
  • [x] If you have a change on the documentation, please link to the page that you change.
  • [x] If you add a new feature please update the documentation in the same PR.
  • [x] If you really need to add a breaking change, explain why it is needed. Understand that this result in a lower change to get the PR accepted.

    I made it non-breaking 😃
  • [x] Any PR need 2 approvals before it get merged, sometimes this can take some time. Please be patient.

Adding a New Rule Property

  • [x] Add the new property to rule set XML, e.g. src/main/resources/rulesets/naming.xml
  • [x] Add documentation for the new property, e.g. src/site/rst/rules/naming.rst
  • [x] Implement new property in rule, e.g. src/main/php/PHPMD/Rule/Naming/LongVariable.php
  • [x] Cover cases for the new property in rule test, e.g. src/test/php/PHPMD/Rule/Naming/LongVariableTest.php
    • [x] Cover the case when the new property is not set and the rule should not apply
    • [ ] Cover the case when the new property is not set and the rule should apply
    • [ ] Cover case when the new property is set and the rule should not apply
    • [x] Cover case when the new property is set and the rule should apply
    • [ ] Cover edge cases of the new property, if any

Comment

I admit I did not look out for contribution guides. I just did. Reviewing your checklist here I think I already covered most of the parts. As I shall be patient I can also later amend my tests. For now you can already get what I want to do and review my approach. I assume I did not match something and I have to adjust anyway 😅

JoshuaBehrens avatar Jun 15 '22 22:06 JoshuaBehrens

@JoshuaBehrens thanks for your PR. The first high over impression is that you did a good job. I have enabled the GitHub actions to run but now I see that they fail, can you take a look to it?

tvbeek avatar Jun 16 '22 06:06 tvbeek

Thank you for the quick feedback. I was not sure we are talking about that level of patience :D Sure. I think I already saw the issue. Test fixture lookup by method name fails because I had some test method renaming going on.

JoshuaBehrens avatar Jun 16 '22 08:06 JoshuaBehrens

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.35% :warning:

Comparison is base (10f8159) 89.10% compared to head (05b31ec) 88.75%.

:exclamation: Current head 05b31ec differs from pull request most recent head 51f12f8. Consider uploading reports for the commit 51f12f8 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #962      +/-   ##
============================================
- Coverage     89.10%   88.75%   -0.35%     
+ Complexity     1160     1082      -78     
============================================
  Files           107       96      -11     
  Lines          2956     2722     -234     
============================================
- Hits           2634     2416     -218     
+ Misses          322      306      -16     
Files Changed Coverage Δ
src/main/php/PHPMD/Rule/Naming/LongClassName.php 100.00% <100.00%> (ø)
src/main/php/PHPMD/Rule/Naming/LongVariable.php 100.00% <100.00%> (ø)
src/main/php/PHPMD/Utility/Strings.php 100.00% <100.00%> (ø)

... and 19 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Sep 10 '22 12:09 codecov-commenter

Is a rebase beneficial for this to get merged? :) I can do that if it helps

JoshuaBehrens avatar Jul 25 '23 18:07 JoshuaBehrens

@JoshuaBehrens this PR was from before I joined the project I think so I wasn't aware of it, thanks for bringing it to my attention. We normally require 2 maintainers to approve a PR before it gets merged and it seems that was all that was missing.

AJenbo avatar Jul 25 '23 18:07 AJenbo

Thank you @AJenbo for taking care of the pull request :) love to contribute again. I have some more ideas

JoshuaBehrens avatar Jul 26 '23 08:07 JoshuaBehrens