Hi Daimona, Thanks for working on this. Have you run this against sniff against mw core or extensions? If so it would be useful to look at examples of what it’s catching in existing code. Kosta

On 2019-02-11 18:42, Daimona wrote: Thanks for that link. I looked at them and submitted a change (please do not merge it) to demonstrate what changes this would require: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/489759 In my opinion most of these changes are clear improvement or harmless, except for the pattern in LBFactorySimple.php/LoadBalancer.php, which is a little tricky and probably clearer in the original version. -- Bartosz Dziewoński

I decided to look at some examples, and I found one that gives me pause.[0] if ( $i == 0 ) { $this->servers[$i]['master'] = true; } else { $this->servers[$i]['replica'] = true; } I don't know what's specifically going on here, but it's possible that only $this->servers[$i]['master'] or $this->servers[$i]['replica'] is ever set (rather than both being previously set to false, for example), so something like this could *possibly* break later code (that would be some brittle code, but worse things have been done): $this->servers[$i]['master'] = ( $i == 0 ); $this->servers[$i]['replica'] = !$this->servers[$i]['master']; I'm not sure how else to refactor this to avoid the pointless conditional failure. That said, thanks for the work to continue to improve our code base! —Trey [0] https://gerrit.wikimedia.org/g/mediawiki/core/+/6968592a9acd683cb7fee4b0f7d… Trey Jones Sr. Software Engineer, Search Platform Wikimedia Foundation On Mon, Feb 11, 2019 at 12:43 PM Daimona <daimona.wiki(a)gmail.com> wrote:

Sorry it took me so long to respond here. In https://gerrit.wikimedia.org/r/486813 Gergő wrote: Thanks! I think this sums it up really well. 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. 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. 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