4 ms·

As usual be attentive and suspicious when using such tools as its suggestions might not be appropriate. For example let's take the following code: with open(

As usual be attentive and suspicious when using such tools as its suggestions might not be appropriate. For example let's take the following code: with open("foo") as a: contents = a.read(100) Running it through refurb produces the following suggestion: > main.py:1:1 [FURB101]: Use `y = Path(x).read_text()` instead of `with open(x, ...) as f: y = f.read()`

dosisod 4y ago
Yes, Refurb error messages are not gospel: Make sure the changes make sense! Thank you for bring this bug up, I will make sure to address it tomorrow.
dosisod 4y ago
Also, I don't know if this was part of your critique, but are you also mentioning that the "a" variable is not in the suggestion? The reason behind that is long names, such as "super_special_file" would greatly increase the length of the error messages, making it hard to see the "meat" of what is being changed. Hence the x and y naming. Perhaps I should add a flag to allow verbatim (or close to it) errors. Recreating the source lines exactly will be pretty hard, but I think we can do better here.
- sfink 4y ago
  Perhaps substitute the actual variables if they are simple and short, otherwise do something like: > main.py:1:1 [FURB101]: Use `y = pathlib.Path(x).read_text()` instead of `with open(x, ...) as f: y = f.read()`, where y=my_important_data and x=super_special_file Btw: thanks, your tool suggested a handful of things I didn't know about, and pushed me towards using pathlib instead of relying on my muscle memory to do the laborious `os.path.join(..., ...)`. One request I would have is to spell out the full name of suggested imports. I had never heard of `contextlib.suppress`. And in the above message, I think it will be obvious that you could do `from pathlib import Path` instead of the literal text in the suggestion.
  - dosisod 4y ago
    I think this would be a good change. I agree that the imported names should be spelled out: You shouldn't have to go hunting down the docs before you go and change anything. Again, this would increase horizontal space, but I think it is for the better (the errors are already pretty long anyways).
iib 4y ago
I looked at the example for some time now, and I don't understand what exactly the problem is. Is it the missing `import pathlib` and then `pathlib.Path...`? Is using `read_text()` bad style?
- capableweb 4y ago
  I'm by no means a Python expert, but as far as I can tell, those two snippets do wildly different things. `.read(100)` would read 100 bytes from the file, `.read_text()` takes the entire file and returns a decoded string. Running the former on a 100GB file will work OK, running the latter on the same file will lead to OOM (unless you actually have +100GB free memory). Basically, they are not equivalent.
  - BerislavLopac 4y ago
    As I understand, the snippets in the suggestions are not supposed to be applied verbatim, but as generic proposals that need to be tweaked for the specific example (or ignored completely). Note that it doesn't say to replace `.read(100)` with `.read_text()`; it uses the default version (without arguments) for both. (That being said, `read_text` doesn't have a limit argument, but that is a completely different issue.) So you're right - if you're applying the suggestion verbatim without thinking, you will get incorrect results; but the suggestions were not intended for that.
    - kevincox 4y ago
      I find that linters that have a non-trivial false-positive rate get very annoying very quick.
  - iib 4y ago
    Yes, I completely missed the `100` parameter to `read`. And as a sibling comment said, I also understand them to be suggestions, but in this case, I think `read()` and `read(param)` should be treated as two separate cases, or at least this caveat made clear.
  - uranusjr 4y ago
Twirrim 4y ago
I'm not sure I understand what value pathlib is providing here. The explanation is to avoid the context manager / multi-line. Why though? What is saving you one line really getting? It's not really improving readability in any way. An alternative one liner could be contents = open("foo").read(100) No need to import another library and use its features. Added to which pathlib has some speed issues. Pathlib has some really neat advantages and should be favoured over most of os library file stuff, but not as a replacement for a simple open.
- Majestic121 4y ago
  Pathlib opens and closes the file. With your version, unless I'm mistaken the file does not get closed
  - kevincox 4y ago
    Python is reference counted and it is very clear in this code that a reference to the file isn't leaked, so the file will be reliably closed at the end of the statement. Some people don't like to rely on the reference counter but I think in simple cases like this where only a temporary is created it is quite safe.
    - rcthompson 4y ago
      > it is very clear in this code that a reference to the file isn't leaked Is it? What if an exception is thrown? See e.g. this SO answer[1]: > However if an exception is thrown [...] then a reference to the file will be kept as part of the stack trace in the traceback object and the file will not be closed, at least until the next exception is thrown. So your code, if called from somewhere else inside a try block, leaks a filehandle potentially indefinitely. [1]: https://stackoverflow.com/a/2404671/125921 https://stackoverflow.com/a/2404671/125921
    - _ZeD_ 4y ago
      CPython relies on reference count. other python implementations, like PyPy, does not.

dosisod 4y ago

Yes, Refurb error messages are not gospel: Make sure the changes make sense! Thank you for bring this bug up, I will make sure to address it tomorrow.

dosisod 4y ago

Also, I don't know if this was part of your critique, but are you also mentioning that the "a" variable is not in the suggestion? The reason behind that is long names, such as "super_special_file" would greatly increase the length of the error messages, making it hard to see the "meat" of what is being changed. Hence the x and y naming. Perhaps I should add a flag to allow verbatim (or close to it) errors. Recreating the source lines exactly will be pretty hard, but I think we can do better here.

sfink 4y ago
Perhaps substitute the actual variables if they are simple and short, otherwise do something like: > main.py:1:1 [FURB101]: Use `y = pathlib.Path(x).read_text()` instead of `with open(x, ...) as f: y = f.read()`, where y=my_important_data and x=super_special_file Btw: thanks, your tool suggested a handful of things I didn't know about, and pushed me towards using pathlib instead of relying on my muscle memory to do the laborious `os.path.join(..., ...)`. One request I would have is to spell out the full name of suggested imports. I had never heard of `contextlib.suppress`. And in the above message, I think it will be obvious that you could do `from pathlib import Path` instead of the literal text in the suggestion.
- dosisod 4y ago
  I think this would be a good change. I agree that the imported names should be spelled out: You shouldn't have to go hunting down the docs before you go and change anything. Again, this would increase horizontal space, but I think it is for the better (the errors are already pretty long anyways).

iib 4y ago

I looked at the example for some time now, and I don't understand what exactly the problem is. Is it the missing `import pathlib` and then `pathlib.Path...`? Is using `read_text()` bad style?

capableweb 4y ago
I'm by no means a Python expert, but as far as I can tell, those two snippets do wildly different things. `.read(100)` would read 100 bytes from the file, `.read_text()` takes the entire file and returns a decoded string. Running the former on a 100GB file will work OK, running the latter on the same file will lead to OOM (unless you actually have +100GB free memory). Basically, they are not equivalent.
- BerislavLopac 4y ago
  As I understand, the snippets in the suggestions are not supposed to be applied verbatim, but as generic proposals that need to be tweaked for the specific example (or ignored completely). Note that it doesn't say to replace `.read(100)` with `.read_text()`; it uses the default version (without arguments) for both. (That being said, `read_text` doesn't have a limit argument, but that is a completely different issue.) So you're right - if you're applying the suggestion verbatim without thinking, you will get incorrect results; but the suggestions were not intended for that.
  - kevincox 4y ago
    I find that linters that have a non-trivial false-positive rate get very annoying very quick.
- iib 4y ago
  Yes, I completely missed the `100` parameter to `read`. And as a sibling comment said, I also understand them to be suggestions, but in this case, I think `read()` and `read(param)` should be treated as two separate cases, or at least this caveat made clear.
- uranusjr 4y ago

Twirrim 4y ago

I'm not sure I understand what value pathlib is providing here. The explanation is to avoid the context manager / multi-line. Why though? What is saving you one line really getting? It's not really improving readability in any way. An alternative one liner could be contents = open("foo").read(100) No need to import another library and use its features. Added to which pathlib has some speed issues. Pathlib has some really neat advantages and should be favoured over most of os library file stuff, but not as a replacement for a simple open.

Majestic121 4y ago
Pathlib opens and closes the file. With your version, unless I'm mistaken the file does not get closed
- kevincox 4y ago
  Python is reference counted and it is very clear in this code that a reference to the file isn't leaked, so the file will be reliably closed at the end of the statement. Some people don't like to rely on the reference counter but I think in simple cases like this where only a temporary is created it is quite safe.
  - rcthompson 4y ago
    > it is very clear in this code that a reference to the file isn't leaked Is it? What if an exception is thrown? See e.g. this SO answer[1]: > However if an exception is thrown [...] then a reference to the file will be kept as part of the stack trace in the traceback object and the file will not be closed, at least until the next exception is thrown. So your code, if called from somewhere else inside a try block, leaks a filehandle potentially indefinitely. [1]: https://stackoverflow.com/a/2404671/125921 https://stackoverflow.com/a/2404671/125921
  - _ZeD_ 4y ago
    CPython relies on reference count. other python implementations, like PyPy, does not.