Better example for W0640: cell-var-from-loop

[SubaruArai]

Type of Changes

	Type
✓	📜 Docs

Description

Add another example for W0640: cell-var-from-loop.
Example to use functools.partial

[codecov]

Codecov Report

Merging #8960 (f960a1f) into main (7b72c39) will not change coverage.
Report is 2 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8960   +/-   ##
=======================================
  Coverage   95.76%   95.76%           
=======================================
  Files         173      173           
  Lines       18639    18639           
=======================================
  Hits        17850    17850           
  Misses        789      789           

[SubaruArai]

I felt this is trivial, so left the news fragment untouched.

[Pierre-Sassoulas]

Thank you for the contribution :) Could you create a good/ directory with two aptly named files inside instead please ? The final result will be more readable. You can look at bad-chained-comparison for an example of what to do.

[Pierre-Sassoulas]

Great first contribution :) ! I'm going to remove the foo/bar in the example directly in this MR, sorry for the noise.

[SubaruArai]

Just updated to keep consistency

[Pierre-Sassoulas]

Locally I get a cell-var-from-loop from the good example, do you also have that ? (cd doc/; pytest -k cell-var)

[SubaruArai]

My bad, a quick look does seem that my code is wrong XD

[SubaruArai]

Fixed the example, but it seems the "bad" directory needs 2 examples.

@Pierre-Sassoulas Any idea to circumvent? Maybe I need to come up with another bad case?

[Pierre-Sassoulas]

I'm going to fix the template to be able to have one bad and two good, it's still very new :)

[Pierre-Sassoulas]

Hello, the problem do not come from the helper for the templates with directory, the good.py example actually raises cell-var-from-loop, do you mind having a look ? I'm going to fix the error message from the test template but it's not super easy to have an accurate message considering we do not have the information of the file raising a message with the current structure of the code.

[SubaruArai]

Sorry, closed by accident

[SubaruArai]

If the intend of the example was:

def greet_teachers(names):
    for name in names:
        def greet():
            # do something
            print(f"Hello, {name}!")
        greet()

Shouldn't the good example be like:

def greet_teachers(names):
    def greet(name):
        # do something
        print(f"Hello, {name}!")

    for name in names:
        greet(name)

Why make the greet function global?

Also, functools.partial will be inappropriate for this purpose.

[DanielNoord]

If the intend of the example was:

def greet_teachers(names):
    for name in names:
        def greet():
            # do something
            print(f"Hello, {name}!")
        greet()

Shouldn't the good example be like:

def greet_teachers(names):
    def greet(name):
        # do something
        print(f"Hello, {name}!")

    for name in names:
        greet(name)

Why make the greet function global?

Also, functools.partial will be inappropriate for this purpose.

I think these examples are very good indeed! You could consider using those?

[SubaruArai]

@DanielNoord While I definitely can, I don't like the fact that the "bad" example works just as fine - albeit less efficient.

What would be a good "bad" example?

[DanielNoord]

I think the bad example is also fine? It's the same as what we currently have in the documentation and it is the code that raises the warning. It's just bad practice to write Python code like that.

[Pierre-Sassoulas]

I noticed that the bad example produce a warning but does not actually produce a problem in what is printed (python bad.py produce the expected list of names). Ideally we would use something that is not a false positive in the doc 😄

[SubaruArai]

It will be quite a lot of change, but what about this:

Bad:

def greet_teachers(names):
    greetings = []
    for name in names:

        def greet():
            # do stuff
            print(f"hello, {name}")

        if name.isalpha():
            greetings.append(greet)

    for greet in greetings:
        greet()


greet_teachers(["Alice", "Bob", "Charlie", "Not-A-Name"])
# "hello, Not-A-Name" * 3

Good:

def greet_teachers(names):
    names_to_greet = []

    def greet(name):
        # do something
        print(f"hello, {name}")

    for name in names:
        if name.isalpha():
            names_to_greet.append(name)

    for name in names_to_greet:
        greet(name)


greet_teachers(["Alice", "Bob", "Charlie", "Not-A-Name"])
# "hello, Alice", ...

[SubaruArai]

And using functools.partial:

import functools


def greet_teachers(names):
    greetings = []
    for name in names:
        if name.isalpha():
            greetings.append(functools.partial(print, f"hello, {name}"))

    for greet in greetings:
        greet()


greet_teachers(["Alice", "Bob", "Charlie", "Not-A-Name"])
# "hello, Alice", ...

But then again, this isn't doing lazy evaluation of string formatting.

[Pierre-Sassoulas]

Sound good ! I was hoping something simpler was possible, but I did not have the time to find a simple example and a working example is paramount.

[SubaruArai]

While this does lazy formatting, pylint will complain to use f-strings:

import functools


def greet_teachers(names):
    greeting_msgs = []
    for name in names:
        if name.isalpha():
            greeting_msgs.append(functools.partial("hello, {}".format, name))

    for greeting_msg in greeting_msgs:
        print(greeting_msg())


greet_teachers(["Alice", "Bob", "Charlie", "Not-A-Name"])
# "hello, Alice", ...

Maybe a candidate for false positive?

[SubaruArai]

I was hoping something simpler was possible

Yeah, I feel that too. The thing is, having this warning mostly means you're abusing lazy evaluation - which is kind of complicated for newcomers anyways.

[SubaruArai]

Ok, I've updated the pr.

[Pierre-Sassoulas]

LGTM, it's blocked by #8991 though, let's rebase on it when it's merged.

[Pierre-Sassoulas]

Thank you for taking the time to make the doc about this message great ! (Also permitted to make the template better in the process 👌 )

[Pierre-Sassoulas]

Congratulation on becoming a contributor, next time you won't have to wait for approval to launch a CI job :)

[SubaruArai]

No, thank you all for the patience!