Fix some ruff errors and simplify LICHESS_TYPE

[AttackingOrDefending]

Type of pull request:

• Bug fix
• Feature
• Other

Description:

Fixes some ruff errors and removes them from being ignored. I'm not sure if some changes (like removing else after return) are beneficial, so I'm open to reverting them back.

Also, makes Lichess in test_bot.lichess inherit from lib.lichess.Lichess to simplify away the LICHESS_TYPE variable.

Related Issues:

None.

Checklist:

• I have read and followed the contribution guidelines.
• I have added necessary documentation (if applicable).
• The changes pass all existing tests.

Screenshots/logs (if applicable):

[MarkZH]

All of the changes are good, except that I agree with your hesitation about changing the if-elif-else structures. Different ways of writing them can have extra meaning to people, even if the Python interpreter treats them the same.

---

(Apologies for the pedantry)

TL;DR: I suggest putting RET505 and RET508 back in the ignore list, but also reviewing the if-blocks it flags for consideration.

---

One thing ruff seems to really like is turning every if-elif-else that has return statements into guard patterns. For example, the following functions are all equivalent:

def average1(numbers: list[float]) -> float:
    if numbers:
        s = sum(numbers)
        n = len(numbers)
        return s/n
    else:
        raise ValueError("Cannot find average of empty list.")

def average2(numbers: list[float]) -> float:
    if numbers:
        s = sum(numbers)
        n = len(numbers)
        return s/n

    raise ValueError("Cannot find average of empty list.")

def average3(numbers: list[float]) -> float:
    if not numbers:
        raise ValueError("Cannot find average of empty list.")

    s = sum(numbers)
    n = len(numbers)
    return s/n

Statements in a sequence are the easiest to understand since they don't have branches or loops. So, we should place the most important parts of a function within the fewest control structures (minimizing indentation).

The first function, average1, puts the normal path on the same footing as the error path, which doesn't seem right since the normal path is way more common.

The second function, aveage2 (that ruff prefers), makes it look like the normal path that calculates the average is the special case (this is the reverse of the guard pattern).

The third function, average3, has the correct form of the guard pattern and is the one I would choose.

In this PR, I would agree with the change to the if-block starting on line 283 of lib/engine_wrapper.py (if self.comment_start_index < 0:). The return -1 is basically an error condition, so this is a correct guard clause pattern.

One change I don't agree with is this (starting on line 352 of lib/engine_wrapper):

        minutes, seconds = divmod(number, 60)
        if minutes >= 1:
            return f"{minutes:0.0f}m {seconds:0.1f}s"
        else:
            return f"{seconds:0.1f}s"

to

        minutes, seconds = divmod(number, 60)
        if minutes >= 1:
            return f"{minutes:0.0f}m {seconds:0.1f}s"
        return f"{seconds:0.1f}s"

Formatting a time duration of more than a minute is not a special case. These two cases are just different ways of handling different data and so should not be put in different structure.

There is one example of an if-block that ruff thinks should be changed, but I disagree with the ruff-suggested change: lines 188 to 194 of lib/engine_wrapper.py: the raise command should be put in the if-block with the condition reversed. The handling of a bad move could then be unindented.

The changes like

if a:
    return a_result
elif b:
    return b_result
else:
    return c_result

to

if a:
    return a_result
if b:
    return b_result
return c_result

are fine.

Use your judgement when choosing structures.

[AttackingOrDefending]

I changed the structures to what I considered most readable.