search

codeclimate / codeclimate-phpmd

Code Climate PHPMD Engine
MIT License
10 stars 12 forks source link

Use stricter fingerprints for some rules #31

Closed maxjacobson closed 7 years ago

maxjacobson commented 7 years ago

We received a report that this engine produces a number of fixed/new issues on pull request comparisons. I looked at an example PR:

https://codeclimate.com/github/yiisoft/yii2/pull/13113

And saw that the issues showing up as fixed/new are these three.

The location reported by the underlying tool, phpmd, is fairly broad. So, for example, the Naming/ShortMethodName rule checks that your method names aren't too short, and reports the location as being from the beginning of the method to the end of the method. This means, with our default fingerprinting algorithm, that if you change the method at all, we'll generate a new fingerprint for the issue, and it will show up as fixed/new.

Some phpmd rules don't include a "name", but I confirmed that these three do.

maxjacobson commented 7 years ago

@codeclimate/review

ABaldwinHunter commented 7 years ago

@maxjacobson I agree about the fingerprinting seeming off in this case.

Looking at those rules, it seems like the name rule location should be where the variable is introduced.

WDYT about changing the location instead?

I guess a downside there would be losing parity with the underlying tool.

If you think that approach would be worse, this change LGTM

ABaldwinHunter commented 7 years ago

Hm on second thought, the change here seems a lot simpler. LGTM

wfleming commented 7 years ago

WDYT about changing the location instead?

I guess a downside there would be losing parity with the underlying tool.

Personally I lead more towards preferring parity with the underlying tool.

I would guess that back-tracking to where the variable is introduced is non-trivial, and in many cases probably an undecidable problem (since PHP has similar semantics as JS in that vars can be introduced wherever they first happen to be referred to, e.g. you don't actually have to declare them, and which reference is "first" could depend on program state.)

maxjacobson commented 7 years ago

Yea, I'm inclined to keep this simple if we can. I can imagine there are other rules that would require more invasive surgery to get a stable fingerprint. But I think it's OK to not worry about them until we need to.