Add prefix removal for class names and variable names #962
Conversation
|
@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 |
JoshuaBehrens
force-pushed
the
feature/prefix-removal-for-name-lengths
branch
from
570a388
to
07048f7
Compare
JoshuaBehrens
force-pushed
the
feature/prefix-removal-for-name-lengths
branch
from
07048f7
to
2d017b5
Compare
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
|
Is a rebase beneficial for this to get merged? :) I can do that if it helps |
| @@ -109,7 +116,11 @@ protected function checkMaximumLength(AbstractNode $node) | |||
| { | |||
| $threshold = $this->getIntProperty('maximum'); | |||
| $variableName = $node->getImage(); | |||
| $lengthWithoutDollarSign = Strings::lengthWithoutSuffixes($variableName, $this->getSubtractSuffixList()) - 1; | |||
| $lengthWithoutDollarSign = Strings::lengthWithoutPrefixesAndSuffixes( | |||
| \ltrim($variableName, '$'), | |||
There was a problem hiding this comment.
Nit: I dont' think we normally name space functions
| \ltrim($variableName, '$'), | |
| ltrim($variableName, '$'), |
|
@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 |
Type: feature / documentation update
Issue: Adds prefix removal for variables and class names
Breaking change: no
Changed documentation pages:
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.
I made it non-breaking 😃
Adding a New Rule Property
src/main/resources/rulesets/naming.xmlsrc/site/rst/rules/naming.rstsrc/main/php/PHPMD/Rule/Naming/LongVariable.phpsrc/test/php/PHPMD/Rule/Naming/LongVariableTest.phpComment
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 😅