Pylint: Maintainability enhancements

[alex4747-pub]

Is your feature request related to a problem? Please describe

1. The meaning of a complex conditional statement may be changed by accidental indentation change. This may introduce hard to find maintenance issues.
2. Python has a very different variable scoping rules from the rest of the world. More and more experienced programmers are using python and we may introduce hard to find bugs related to unfamiliar scoping behavior.
3. There are several features that make python convenient to type in a program quickly, but the same featured make it hard to maintain (args, kwargs, named parameters). With maturing of the environment maintenance costs will far overweight development costs.

Describe the solution you'd like

1. A complex solution is to optionally flag complex conditionals. A simple solution is optionally require 'else' for every 'if'
2. If variable is used in more than one scope, require it to be declared at the top of the function (or before the first use)
3. Add flags disabling use of features that lead to poor maintainability.

Additional context

Programming for 40+ years, professionally 30+, worked on very large system with 20+ years lifetimes, for a successful company cost of unmaintainable code is counted billions of lost opportunity, because it could not move fast enough.

[hippo91]

@alex4747-pub thanks for this proposal. I'm afraid that at least the two first points are contradictory with python idioms.
For example pylint will emit a message no-else-return in the following case:

if a > 5:
    return True
else:
    return False

because it is recommended to write:

if a > 5:
    return True
return False

It seems to be in total opposition to what you propose.

It could be easier to discuss around if you open an issue for each enhancement separately with example code.

[alex4747-pub]

I did not want to add too many clauses - no else should be used (no else should be required) after return and continue.

I was thinking about it and maybe pylint can just flag complex conditionals - it could be easily resolved by developer e.g. by defining a local function without need for 'else pass'

[hippo91]

@alex4747-pub can you give an example of problematic code and the output you expect from pylint?

[alex4747-pub]

Sorry for late reply was really busy. Below are two variants of the same code. It will be nice if the first one was optionally flagged by pylint

# Change in indentation of the last elif
# changes the meaning and hard to detect

if old.config:
    if not new.config:
        machine.disable()
    elif old.config.value != new.config.value:
        machine.disable()
        machine.enable(new.config.value)
elif new.config:
    machine.enable(new.config.value)

# Change in indentation will raise  a syntax error
if old.config:
    if not new.config:
    machine.disable()
    elif old.config.value != new.config.value:
    machine.disable()
    machine.enable(new.config.value)
    else:
        pass
elif new.config:
    machine.enable(new.config.value)

[hippo91]

@alex4747-pub i now see clearly what you are asking. Thanks.
One solution would be to flag all if statements that do not end explicitly.
However it would then push people to add else: pass statements and doing this may lead to lesser readability as stated in https://stackoverflow.com/questions/22865572/is-it-best-practice-to-include-an-else-pass-statement-at-the-end-of-a-python-if
As you said, we should probably furnish this check as an option disabled by default.
@AWhetter @Pierre-Sassoulas @brycepg what do yo you think about it?

I just attach a simple snippet (based on the example of this issue) which could be used a unit test if needed:

#pylint: disable=missing-module-docstring, missing-function-docstring
def check_config(machine, old_conf, new_conf):
    if old_conf:
        if not new_conf:
            machine.disable()
        elif old_conf.value != new_conf.value:
            machine.disable()
            machine.enable(new_conf.value)
    elif new_conf:
        machine.enable(new_conf.value)

[alex4747-pub]

Yes, it should be configurable option. I suppose it may require else for nested/inner if statements before else aor elif of outer if statements. Once you have a prototype it will be interesting to see how often this issue is hit on some existing code base.

[Pierre-Sassoulas]

I understand the concern, but I don't like this else: pass solution. IMHO the target code using your example should be this:

```python
def check_config(machine, old_conf, new_conf):
if old_conf:
check_old_config(machine, new_conf, old_conf)
elif new_conf:
machine.enable(new_conf.value)

def check_old_config(machine, old_conf, new_conf):
if not new_conf:
machine.disable()
elif old_conf.value != new_conf.value:
machine.disable()
machine.enable(new_conf.value)
```

So what about a message named confusing-consecutive-elif "consecutive elif with differing indentation level, consider creating a function to separate the inner elif" ?

[alex4747-pub]

I am not insisting on 'else: pass' solution and I did consider writing nested function for proper separation myself. The important part is to have it flagged. The message seems ok to me.