Do not suppress warnings in PHP_CodeSniffer reports

[morozov]

Suppressed warnings confuse contributors since the report doesn't contain all violations which a pull request may contain.

Q	A
Type	improvement
BC Break	no

See #3217 (comment) for the reference.

[Majkl578]

Hmm, interesting gotcha, but I wouldn't say it's a configuration issue per se.

I'd instead prefer different approach: leverage the warnings we are considering strict part of our CS into errors, like this:

<rule ref="PSR2.Methods.MethodDeclaration.Underscore">
    <type>error</type>
</rule>

[morozov]

@Majkl578 IMO both approaches are valid and mutually complementary. If by default a warning doesn't fail the build (not sure if that's the case), it still should be possible to refer to the log which would not suppress warnings.

E.g., a line length violation is a warning. Maybe it shouldn't force developers to fix it by any means (e.g. by slicing a long string literal) but if it's an expression which can be reformatted into multiple lines, the code style report should encourage doing so by showing warnings.

[Majkl578]

It does fail (exit code 1) unless you specify an option: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#ignoring-warnings-when-generating-the-exit-code

[morozov]

It does fail (exit code 1) unless you specify an option

Hm… maybe we should use this option? Not all warnings can be fixed w/o crippling the code or adding annotations (both is undesired). Or let's just enable the warnings first and see how annoying they turn out to be. Suppression is a no-go in any event.

[morozov]

I'd instead prefer different approach: leverage the warnings we are considering strict part of our CS into errors […]

This is already done by having the build fail in case of a warning. There's a reason to not promote "MethodDeclaration.Underscore" to an error. Semantically, an error is what you must fix while a warning is what you should fix if it's possible (my own interpretation).

If there's an existing protected method with an underscore which you override, there's no way to not introduce this error in code which adds or modifies the line with the overriding method. And it doesn't seem to be going to change according to #3007 (comment) (which I don't completely agree with).

[Majkl578]

I don't think it's a good idea. Some warnings are just annoying and if we don't enforce them, what's the point of having/seeing them?

Not all warnings can be fixed w/o crippling the code or adding annotations (both is undesired).

This is also true for our other sniffs that throw errors, i.e. declare(), parameter/return types, naming conventions etc.

Or let's just enable the warnings first and see how annoying they turn out to be.

This is how annoying it would be in ORM: https://gist.github.com/Majkl578/afcd89f4501c56ea5866142be6d0675c

I also disagree with #3007 (comment) btw.

[morozov]

Some warnings are just annoying and if we don't enforce them, what's the point of having/seeing them?

The point is seeing violations and fixing or asking contributors to fix at least those which is possible. And having a better readable codebase in the end result. Look at #3217 (comment), this is how much frustration it causes to explain the need of a change if the code violation is not flagged by CI and figuring out why that is.

This is how annoying it would be in ORM: https://gist.github.com/Majkl578/afcd89f4501c56ea5866142be6d0675c

How many of them are not fixable? Given our incremental approach, how often will non-fixable violations be introduced or touched? Personally, I find it more annoying to have to scroll horizontally or resize my editor window to accommodate long lines — that's what the requirement is for in the first place.

This is also true for our other sniffs that throw errors, i.e. declare(), parameter/return types, naming conventions etc.

I don't know what those sniffs are since I haven't even seen their errors in DBAL because of suppression. Maybe if you provide some more examples, they could help me see your point better.

[morozov]

@Majkl578, ping. Why don’t we enable warnings and then adjust the standard if it turns out some rules don’t bring value?