Re: [Wikitech-l] Code sniff for verbose conditionals

2019-03-01 Thread Thiemo Kreuz
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!

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-11 Thread Trey Jones
I see that my case has already been found by Bartosz, so disregard my message. Sorry! On Mon, Feb 11, 2019 at 3:36 PM Trey Jones wrote: > 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->serv

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-11 Thread Trey Jones
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->serv

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-11 Thread Daimona
Thanks for the change! I just realized that my patch has another problem: when checking assignments, it should also check that the receiver is the same in both branches, and this would avoid the case in LoadBalancer. I also find excessive the case in GlobalFunctions, and that would be solved by imp

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-11 Thread Bartosz Dziewoński
On 2019-02-11 18:42, Daimona wrote: Hi, All patches in the codesniffer repo have a sample run against mwcore set up in CI. As can be seen in [0], the current version is triggered 13 times by MW core. No idea about extensions, though. Daimona [0]: https://integration.wikimedia.org/ci/job/mw-tools

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-11 Thread Daimona
Hi, All patches in the codesniffer repo have a sample run against mwcore set up in CI. As can be seen in [0], the current version is triggered 13 times by MW core. No idea about extensions, though. Daimona [0]: https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/966/consol

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-11 Thread Kosta Harlan
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 Feb 9, 2019, at 9:26 PM, Stas Malyshev wrote: > > Hi! > >> I was saying, you can find severa

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-09 Thread Stas Malyshev
Hi! > I was saying, you can find several examples of wrong and correct code at > [1]. > Thiemo pointed out that this patch could encourage to write shorter and > less readable code (you can find his rationale in Gerrit comments), and I > partly agree with him. My proposal is to only trigger the sn

Re: [Wikitech-l] Code sniff for verbose conditionals

2019-02-09 Thread Daimona
(Pardon, I hit "send" while typing :/) I was saying, you can find several examples of wrong and correct code at [1]. Thiemo pointed out that this patch could encourage to write shorter and less readable code (you can find his rationale in Gerrit comments), and I partly agree with him. My proposal

[Wikitech-l] Code sniff for verbose conditionals

2019-02-09 Thread Daimona
Currently, there's a patch under review [0] to enable a PHP sniff which would detect so called "Pointless conditionals". These are conditionals (if...else or ternary) where both branches only assign or return a boolean value, and which could potentially one-lined using the if condition. T _