Add prefix removal for class names and variable names
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 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?
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.
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%> (ø) |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Is a rebase beneficial for this to get merged? :) I can do that if it helps
@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.
Thank you @AJenbo for taking care of the pull request :) love to contribute again. I have some more ideas