Implement LongMethodName

[kylekatarnls]

Type: feature
Issue: Resolves Fix #486
Breaking change: no

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4a75765) 92.53% compared to head (9e37925) 92.58%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1045      +/-   ##
============================================
+ Coverage     92.53%   92.58%   +0.04%     
- Complexity     1239     1244       +5     
============================================
  Files           111      112       +1     
  Lines          3404     3424      +20     
============================================
+ Hits           3150     3170      +20     
  Misses          254      254              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ravage84]

Thank you very much for the new rule.

Reviewed code-only, didn't test it.

[ravage84]

I don't think list is supported by all API doc tools:

https://docs.phpdoc.org/3.0/guide/guides/types.html#supported-types

[kylekatarnls]

list is now supported for years by PHPStorm, Plsam, PHPStan and Phan, it adds an interesting precision that keys of an array $value are strictly equal to range(0, count($value)-1) (I.E. being ordered integer sequence starting at 0, see array_is_list). Such as the result of explode, array_values, array_keys.

IMO giving the benefit to the majority of tools who support it worth to drop other tools supports (which should fallback if they don't know how to read the annotation: to array if the return type is specified, else to mixed).

[ravage84]

This is quite a lot of code to replace a few lines previously. Not sure what to feel about it...

[kylekatarnls]

This will make getExceptionList() to disappear completely in the next version and we can simply make exceptions available and working the same way for all the rule.

So on the next version it's more feature with way less (redundant) code.

[kylekatarnls]

And introduce it now creates an easier step for people who created custom routes to prepare for upgrade.

[kylekatarnls]

It would need more discussion as it's a bit more opinionated but other types of config could also be shared so to make them also working the same way across different rules and auto-discoverable and auto-documented.

For instance with attributes (but would need to drop PHP <= 7):

class LongMethodName extends AbstractRule
{
  #[ConfigProperty(description: 'Comma-separated list of exceptions')]
  protected ExceptionProperty $exceptions;

  #[ConfigProperty(description: 'Maximum length for a method or function name')]
  protected Threshold $maximum;
}

Without dropping PHP version, it can be dependency-injection:

class LongMethodName extends AbstractRule
{
  public function __construct(
    /** Comma-separated list of exceptions */
    ExceptionProperty $exceptions,

    /** Maximum length for a method or function name */
    Threshold $maximum,
  ) {
  }
}

[ravage84]

Isn't there any less technical name to express what we actually want to achieve or what we actually get back from this method?

Also, not having doc blocks, doesn't help either. 😞

[kylekatarnls]

array_flip is a micro-optimization so index and isset() can be used instead of in_array(). However if we plan to support pattern everywhere we allow exceptions, then we would no longer need it in next major version.

[kylekatarnls]

Taking a look at https://github.com/phpmd/phpmd/actions/runs/7159958473/job/19493792510?pr=1045#step:9:1769

I guess 20 might be a little short for a default value.