Add rulesets and rules properties

[ichiriac]

On order to filter results or to add statistics, it would be cool to have a sort of ruleset and rule property on each inspection item a similar thing as https://phpmd.org/rules/index.html

[DaGhostman]

Hmm.. Seems that its a good improvement, but also would be nice if there are filters to tell the tool which type of inspections to run, say I care about the clean code more than I care about the code size, but I do care about unused (this is just as an example I will not be looking at this stage to do complex analysis to determine if code is used or not, just yet.. although everything after non-conditional or not in catch block, return and exit is dead code )

Which kind of inspection would be more better to give priority over which? And which will be too broad/out of scope?

[ichiriac]

IMHO dead code and unused variables are important in order to keep clean code (https://phpmd.org/rules/index.html#unused-code-rules)

[ichiriac]

also dangerous functions like eval, print_r or var_dump, but you should be able to disable checks with an annotation like @ignore rulset.rule :

/**
 * @ignore clean.*
 */
function foo() {
  // @ignore clean.danger
  var_dump($debug);
  // @ignore clean.danger
  eval($debug);
}

[DaGhostman]

Hmm.... Wouldn't annotation-based flags bloat the code?

Since you can't really detect if the code does things that one should actually refactor, like if somewhere I am using eval and I disable it like in the example you gave, later when looking for refactoring I can't really know if that eval is there and could lead to unexpected behaviour as code changes. Or maybe allow that kind of suppressing of inspections, but implement a zealous/strict mode which ignores and reports said suppressions as warnings?

[ichiriac]

like in eslint : http://eslint.org/docs/user-guide/configuring.html#configuring-rules

You could provide a way to disable locally a rule just because the error is voluntary, and no refactoring will be provided.

That's said, you can also implement a strict mode that ignores disabling checks.

Also as you can see in eslint, there is various levels of errors : notice, warn, error (+off)

[DaGhostman]

As a personal opinion it is a bad idea to be able to provide custom priorities to iny sort of code quality tool, since that leaves room for circumvention of said rules (which does not make them as much rules, but rather suggestions/recommendations).

Sticking to the example with eval from earlier and assuming custom levels can be set, if a project defines the use of eval to be critical, but then a dev comes in and decides he wants to use eval and provides // @ignore clean.danger the inspection process will ignore it and that might cause problems in the future and eliminate the benefit of the previously defined rules

---

Now leaving my personal take on this, I see that it is a good-to-have feature, but that way of overwriting should not be the default behaviour IMO, but rather have it behind a flag as to ensure that those using it are absolutely aware of the possible consequences that may come.

• It will lower the number of operations when not used (since in-code comments will not have significance and will not have to be always processes)

[ichiriac]

Just take a real-life example :

• Just lets say I'm forced to eval something, up to me to ensure that's correctly secured or escaped.
• So I'm also using a CI tool (like travis) and in my validation tools I'm using php-inspections to ensure my code is clean

So my commit will break the build. I don't want to change my method (for any reasons, like time is money and so on...) - so remains 2 alternatives :

• I can commit some annotation that only this line don't have to be checked (an maybe also add a comment why)
• I can disable the tool because it breaks the build, so I will only use it locally

After using it locally, many errors are expected and I consider them as normal because that's my level of requirements on this project. So I'll be used to have many errors, except the day when a true error will come, I will not be able to see it because results are too verbose...

So that kind of feature is very important because it depends on the needs of developers and needs of projects so you can only provide checks, and pre-defined configurations, but you may also provide a way to disable some specific tests (globally, or just locally in a portion of code)