WPS332 Found implicit ternary expression on boolean expression

[sponsfreixes]

After reading https://wemake-python-stylegui.de/en/latest/pages/usage/violations/consistency.html#wemake_python_styleguide.violations.consistency.ImplicitTernaryViolation, I don't understand why the following code:

if first_condition and second_condition or third_condition:
    do_stuff()

triggers WPS332. The closest related issue I could find is #604. On that example, it is clear that it's a ternary expression to make an assignment. But applying that same reasoning to any boolean logic that combines ands and ors like the example from above does not seem adequate to me.

The only way I found around it is with the construct:

trigger_do_stuff = third_condition if first_condition else second_condition
if trigger_do_stuff:
    do_stuff()

Which strikes me as confusing and not readable.

I would appreciate an explanation of how that boolean logic should be correctly expressed according to WPS, or if this case shouldn't be flagged by 332 (on that case, I would open a bug ticket).

[sponsfreixes]

I've been thinking a bit more about it, and found about example. It feels incorrect that:

if first_condition and second_condition or third_condition:
    do_stuff()

raises WPS332 but

if first_condition and second_condition or third_condition or fourth_condition:
    do_stuff()

is OK.

On both cases we are just applying basic boolean operands. Having 3 terms raises that error but having more than 3 does not.

[sobolevn]

This is a bug. This code should not raise any violations:

if first_condition and second_condition or third_condition:
    do_stuff()

We must check that our potential "implicit ternary expression" is not inside if condition.

Are you willing to fix it? Should be rather simple!

[sponsfreixes]

I'll give it a try on the following weeks!

[sobolevn]

Awesome, thanks!

[sponsfreixes]

I've been thinking about the right way of telling if an expression is used as ternary expression or boolean logic. Only checking for an if would mean that this is correct:

if (product_too_expensive and we_are_low_on_budget) or we_have_no_money:
    reject_product()

But this would be marked as incorrect:

cant_afford_product = (product_too_expensive and we_are_low_on_budget) or we_have_no_money
if cant_afford_product:
    reject_product()

(Parenthesis added on both examples just to emphasize the order of operations, they are actually not needed)

The second example seems to me more readable on some cases, as it is naming the logic, providing meaning to it. But the first one would be marked as preferred by 332 after my changes.

I've been thinking that maybe we should be checking if the operands are booleans, so that we would accept both examples. How does that sound?

[sobolevn]

I've been thinking that maybe we should be checking if the operands are booleans, so that we would accept both examples. How does that sound?

This will involve type inference. And it goes against our rules. So, it is not an option.

How about disabling this rule at all? We can do it in 0.14 release.
I realise that I don't like it at all. It was just a blind port from pylint which is not really helpful.

[sponsfreixes]

I'm fine with disabling the rule. I see value on it, but I think it's too context specific to be able to detect it properly.

Should I make a PR removing it then?

[sobolevn]

Yes, we can merge it into 0.14
Thanks a lot!

[sponsfreixes]

What would be the right way to disable an existing rule? I've checked on the docs, including the Visitors API and the Violations API if there was a way to mark a violation as disabled, but didn't see anything.

My understanding is that I shouldn't just delete all the code related to that violation, because that would fail the test_no_holes test (as there would be a gap on the rules numbering).

Is the right approach to just remove the violation check from the visitor, and modify the tests to reflect that?

[sobolevn]

Just delete the code and tests, that's it.

[sponsfreixes]

I tried that, but it makes test_no_holes fail. If I understand the test correctly, it checks that the error codes are contiguous, with "no holes". So if 332 is missing between 331 and 333, it fails.

What do you thing of adding somewhere a list of deprecated codes that have been removed from the codebase, and test_no_holes uses this information to ignore these codes?

All the other tests pass.

[sobolevn]

Yes, this is a good idea. I am pretty sure that we would need that in the future.

[sponsfreixes]

I almost have it, except for one thing. Are we OK on ignoring rule 442 on our conftest.py files? See discussion on #1120 for context. I am currently infringing it because of a chain of fixtures.

I can add that configuration to my PR

[sobolevn]

Yes, we are ok with it 👍