[Adjust Rule] SIM113 False Positive in Nested Loops

[Skylion007]

Desired change

SIM113 False Positive in nested loops

In this case, we could using enumerate is rather tricky. How do you suggest we best do it in nested loops like so?

• Rule(s): SIM113
• Adjustment: In which way should it/they be adjusted? The result should not be triggered in nested loops

Explanation

Why is the adjustment necessary / better? Because the enumerate logic actually becomes MORE complicated in this nested loop context. You need to have some complex multiplication to get the proper number and that may not even be possible if bars is a generator that has variable length. In which case, counting manually here is actually for the best.

Example

This is an example where the mentioned rule(s) would currently be suboptimal:

count = 0
for foo in foos:
    for bar in bars:
        count +=1

In this second one, I could se the argument for epochs (assuming there is not complex increment logic, but it complains about number of iterations)

epoch, i = np.load(resume_file)
while epochs < TOTAL_EPOCHS:
    for b in batches:
        i+=1

[MartinThoma]

Good point! Thank you for raising it.

At the moment, I'm not sure how to fix it. Or if there is any way to fix it. I'm thinking about removing SIM113 completely again.

For now, you can set --ignore=SIM113 or within the setup.cfg

[flake8]
ignore=SIM113

[GitRon]

Just gave your package a shot and its really nice - but I didn't get the SIM113 (which is not documented on the main page btw). Whats the reason not to use a regular var as a loop counter?

Thanks!

[MartinThoma]

@GitRon Thank you :-)

SIM113 should bring awareness to people who are not aware of enumerate. The advantage of using enumerate (when possible) is that you don't have to increment that variable in the loop body. It is a tiny bit easier to read as the reader does not have to figure out where / when the loop variable is incremented (or if it is incremented in multiple places).

Of course, Skylion007 gave a good example with

count = 0
foos = [1, 2, 3, 4]
bars = ["A", "B", "C"]
for foo in foos:
    for bar in bars:
        count +=1

where this rule should not be triggered.

By the way, it is possible to use enumerate here as well:

count = 0
foos = [1, 2, 3, 4]
bars = ["A", "B", "C"]

from itertools import product
for count, (foo, bar) in enumerate(product(foos, bars)):
    print((count, foo, bar))

But that is harder to read, especially when more happens in the function body.

I'm currently wondering if checking for nested loops via the introduced parent attribute solves the issue.

[GitRon]

Thanks for the explanation! Actually I didn't know about this as well. Looks fancy but I might argue for the simple solution to be valid as well. Linting is quite strict so IMHO I think it should only point out things, you should never do. And if you want to keep a complicated case simple, it might be valid to to for approach one, won't it?

[MartinThoma]

It depends on the exact case. For the example given in this issue by Skylion007 , I agree that NOT using enumerate is the simpler solution.

In the following case, I want to encourage people to use enumerate:

# Bad, SIM113 should be triggered:
i = 0
for element in some_iterable:
    ...   # do something with i
    i += 1

# Good
for i, element in enumerate(some_iterable):
    ...  # do something with i

[GitRon]

Ok, true, this might be a nicer way for the simple case 😃

You might want to add this example to the main Readme so people know in one second how to fix this linting issue.

[MartinThoma]

@GitRon Done :-) I've also linked the issues in which the rules were introduced and renamed the titles of the issues so that they are easier to find. Additionally, I've added a short description of the rule + a link to the example later in the document. I might move the complete set of rules and examples to another page in the future as I'm not a big fan of huge READMEs.