Implement std::ops::Not for ShouldRun

[tsoutsman]

Objective

• Add the negation operator to ShouldRun

Reason

Currently, two functions are required when using binary run criteria, like in this example from the unofficial cheat book. The negation operator allows you to perform the task with just one function, reducing duplicate code and logic.

[mockersf]

Could you go on #2373 and mention if you're ok with the license change?

[tsoutsman]

Done

[DJMcNab]

Actual implementation looks good, I'm just less sure about whether this should use the Not operator.

[DJMcNab]

I'm not certain that this should be the Not operator. I think an associated function called 'invert' or something might make more sense.

(Also I don't think rust-analyzer jumps to Not implementations yet (requires at least rust-lang/rust-analyzer#4558 iirc), so the weird factor is quite high and it's not very inspectible)

[tsoutsman]

Could it have both and then one just calls another?

[Ratysz]

The negation operator allows you to perform the task with just one function, reducing duplicate code and logic.

Can you please elaborate on that? I don't see how this syntax would affect the linked example.

[tsoutsman]

Can you please elaborate on that? I don't see how this syntax would affect the linked example.

Instead of:

fn run_if_connected(
    mode: Res<MultiplayerKind>,
    session: Res<MyNetworkSession>,
) -> ShouldRun
{
    if *mode == MultiplayerKind::Client && session.is_connected() {
        ShouldRun::Yes
    } else {
        ShouldRun::No
    }
}

it could just be:

fn run_if_connected(
    mode: Res<MultiplayerKind>,
    session: Res<MyNetworkSession>,
) -> ShouldRun
{
    !run_if_host(mode)
}

which contains the logic in just one function instead of duplicating it in two. It's not that big of an improvement, but it would make more of a difference for bigger systems. It also makes it clear that this is the exact opposite of the other function.

[Ratysz]

I see now. Still, there is a problem with applying binary operators to non-binary values: lack of clarity with regards to which specific states are being operated on. As in, in a different context the opposite of NoAndCheckAgain is No, and there is no way to tell whether it is !No == NoAndCheckAgain or !No == Yes without looking at the source code.

I don't think that being explicit even in an example this small is a loss:

fn run_if_connected(
    mode: Res<MultiplayerKind>,
    session: Res<MyNetworkSession>,
) -> ShouldRun {
    match run_if_host(mode) {
        Yes => No,
        No => Yes,
        _ => unreachable!(),
    }
}

If this comes up a lot in a project, factor the inversion out into a function; an operator is too ambiguous to be included by default.

I should clarify: I'm going by your last message here. In the linked example run_if_connected() and run_if_host() are not negatives of each other.

[tsoutsman]

I see now. Still, there is a problem with applying binary operators to non-binary values: lack of clarity with regards to which specific states are being operated on. As in, in a different context the opposite of NoAndCheckAgain is No, and there is no way to tell whether it is !No == NoAndCheckAgain or !No == Yes without looking at the source code.

Implementing @DJMcNab's suggestion of an invert method would make it clearer, although I agree that this could very easily be implemented in a project. Then there wouldn't be any confusion or potential for misinterpretation.

I should clarify: I'm going by your last message here. In the linked example run_if_connected() and run_if_host() are not negatives of each other.

My bad, I probably should have properly read the example, but you can imagine a similar example where they are negatives.

[alice-i-cecile]

As much as I want this, I don't think it's feasible unless we swap to a binary logic for ShouldRun. I'm going to close this out; we'll reimplement this ASAP if and when that happens <3