refurb icon indicating copy to clipboard operation
refurb copied to clipboard

[Bug]: Missing FURB151 (`Path(x).touch()`) when separating open() and close()

Open opk12 opened this issue 1 year ago • 2 comments

Has your issue already been fixed?

  • [X] Have you checked to see if your issue still exists on the master branch? See the docs for instructions on how to setup a local build of Refurb.
  • [X] Have you looked at the open/closed issues to see if anyone has already reported your issue?

The Bug

The following code:

#! /usr/bin/env python3
import os.path

path = "/tmp/tmpfile1"

if not os.path.exists(path):
    # Missing FURB151: No warning
    f = open(path, "w")
    f.close()

    # Warns as expected
    open(path, "w").close()

Does not emit the Path.touch() warning for lines 8-9. It only warns for line 12.

$ refurb --enable-all a.py 
a.py:6:8 [FURB141]: Replace `os.path.exists(x)` with `Path(x).exists()`
a.py:12:5 [FURB151]: Replace `open(x, "w").close()` with `Path(x).touch()`

Version Info

Refurb: v1.27.0
Mypy: v1.8.0

Python Version

Python 3.11.7

Config File

# N/A

Extra Info

None

opk12 avatar Feb 08 '24 14:02 opk12

Thank you @opk12 for opening this! This would be a good feature to add.

Out of curiosity, is this example from code you've seen/wrote? I'm trying to find examples of it in the wild using grep.app but it doesn't seem like a very common idiom.

dosisod avatar Feb 09 '24 06:02 dosisod

(edited) Thank you for looking into it. I just wanted to throw an idea in the air, but I don't know if this is a common idiom in real-world code. I saw it in a student homework, which touches the file and then reads it.

def apri_file(nome):
    if not os.path.exists(nome):
        f = open(nome,"w")
        f.close()
    with open(nome, 'r') as route:
        for row in csv.reader(route, delimiter="\t"):
            ...

This example could be refactored to not touch:

def apri_file(nome):
    if os.path.exists(nome):
        with open(nome, 'r') as route:
            for row in csv.reader(route, delimiter="\t"):
                ...
    else:
        special case...

but as the warning is already implemented, I thought it just needs to be extended.

opk12 avatar Feb 09 '24 09:02 opk12