Sorry it took me so long to respond here.
In
https://gerrit.wikimedia.org/r/486813 Gergő wrote:
[…] it adds some fairly complicated code for detecting
a pattern that's mostly harmless and can be dealt with during normal code review, so
the value is less than the maintenance cost.
Thanks! I think this sums it up really well.
we should aim for a threshold which would only reject
code that is clearly wrong […]
See, that's the problem: I don't think a single of the examples I have
seen so far is "clearly wrong". I don't think there is anything
"wrong" with explicitly stating all possible return values. Yea, one
*might* consider some of the examples a little tooo expressive. Some
of them *can* be shortened. But when this is the case and when not is
more an opinion than anything, and needs to be talked about in a code
review.
[…] doesn't mean that we should avoid dealing with
it without even trying.
I don't think this question was answered yet: What are we even trying
to solve here? How big of an issue is this? How often do we come
across such code during our code reviews and feel "I wish a sniff
would have found this before I did"? I do a ton of code reviews. Yes,
there is code like this. But from my personal experience this is so
rare and so much a matter of personal preference and opinion that it's
not worth dealing with the maintenance costs a fully-automated sniff
comes with.
Could you provide specific examples where my proposed
approach produces an undesirable outcome? That is, an example of code you believe should
be acceptable but would be marked as fixable?
I'm sorry, but I believe it should not work this way around. I would
like to point to the examples in the test file. Sure, some of them
*can* be rewritten in a shorter way. But none of them *must* be
rewritten.
CodeSniffer rules are not to make suggestions (actually they are, but
that's not how we use them). The moment we make a sniff report certain
patterns, somebody will come and try to "fix" it, no matter how much
sense it makes. Not long ago a sniff complained about uncommented
@param tags, and people started adding cruft like `@param User $user
The user object`. I would like to avoid running into the same
situation again when we have much more pressing problems, e.g. bad
test coverage, or God objects with hundreds of static methods and
several thousand lines. *These* should make a sniff fail that forbids
making code too complex.
Kind regards
Thiemo