Show HN: Refurb – A tool for refurbishing and modernizing Python codebases
github.com
github.com
seems like Scala is moving to more Python like and Python is moving to add some features from Scala.
One of the other cool features about this project is how the check() functions are loaded based off the type of the node argument:
https://github.com/dosisod/refurb/blob/master/refurb/loader....
This also supports type unions, so you can register your check for binary and unary expressions by typing node as `BinaryExpr | UnaryExpr`.
Why specify all the specific fragments of mypy.nodes, instead of import *?
Docstrings in the middle of code should just die 1/2 of the screen is wasted
Why use a case statement if it only has one match?
I can't even figure out what lines 43-55 do, but that's a lot of nesting
with open(_) as f:
_ = f.read()
Very interesting…
One of the projects I occasionally poke at is a burs (bottom-up rewrite system) generator used to do cost-based tree rewriting. Think this is a concept that might make sense in that context, just need to find the time to play…
glob imports are discouraged as it is hard to figure out where a name comes from (or if it’s just a typo).
> Docstrings in the middle of code should just die 1/2 of the screen is wasted
You mean the class level doc string? That shows up in the help of that class, so there is nowhere else to put it.
I also don’t see the point of a match with a single case.
Line 43-55 is the scrutinee (pattern) of the case statement. It only binds values to the given variables and enters the block if `node` matches the quite complex structure shown there.
The alternative would be doing something repetitive like this:
if not isinstance(node, WithStmt):
return
expr = node.expr
if not len(expr) == 1:
return
if not isinstance(expr, CallExpr):
return
callee = expr.callee
if not (isinstance(callee, NameExpr) and callee.name == 'open'):
return
... etc ...
if isinstance(node, WithStmt) AND
(len(node.expr) = 1) AND
isinstance(node.expr, CallExpr) AND
isinstance(node.expr.callee,NameExper) AND
(node.expr.callee.name == 'open') then
begin
do stuff here
end;It's a very elegant pattern match on a deeply nested structure. It's declarative, not imperative, so if you are concerned about "nesting complexity", you need not be.
How would you implement this otherwise?
There's too many layers of nesting, in too foreign a syntax for me to even parse.
I suspect it could be done a clearer manner in pascal.
[0]: https://youtu.be/ASRqxDGutpA?t=470 [1]: https://github.com/dosisod/refurb/blob/master/refurb/checks/...
Instagram and Dropbox, both used Python.
What I see happening is Python getting "improved to death". As people keep adding "features", aka complexity, to Python it's moving away from its niche and coming into more direct competition with other languages and ecosystems (e.g. Rust, Go, Nim, Haskell, OCaml, even Ada and Java!)
From my POV this tool, though technically very neat, does unnecessary work to make things harder to understand.
Try to see it this way: all this Red Queen's Race, this running hard to stay in the same place, the endless "improving" of languages like Python and JS, are an attack on your knowledge and skills: just sitting there you are becoming obsolete not because your knowledge is degrading but because the youngsters keep changing the tune forcing you to learn new dances just to stay relevant.
There is very little new under the sun in IT: things like type checking and inference are decades old.
I have decades of experience, having shipped code in swift, obj-c, Java, Scala, closure, ocaml, C, C++.
A few weeks ago I was asked to write something substantial in python. I have used python in the past for scripts of < 10 lines, so the syntax is not alien.
I have been extremely impressed by the experience. Async code just works and is very efficient. The match statement is very nice. Interacting with the OS is intuitive and well supported. I’m hard pressed to think of a language that would be more concise for what I’m doing.
I’ve had the same experience with JS recently too. It’s a good language now, and it decidedly wasn’t prior to ES6.
If I wind back 10 years or 20 years, there is just no way the languages as they were as capable as they are now. I can do much more with much less code.
The languages may be more complex, but they enable my code to be less complicated, and me to get more done.
Bonus fact: You can use set iteration in for loops as well! This has the added benefit of sorting the values as well:
>>> for x in {2, 1, 3}: ... print(x) 1 2 3
You didn't ask for that, but I felt like sharing it, to there you go
I don't think you can rely on this? Happy to be proven wrong, but set does not guarantee iteration order. There may be a distinction if all elements are available at set construction, but that seems like a fiddly rule I would rather avoid.
>>> [i for i in {9,40,49}] [40, 9, 49]
Also, you can make anything iterable in python if you implement __iter__.
I learned this when a unit test that iterated over a set started failing sporadically :P super confusing bug!
This is because the string hash function is randomized on process startup to help prevent DOS attacks: https://stackoverflow.com/q/30585108/1517969
>>> for x in {2, 1, 3}: print(x)
1
2
3
>>> for x in {20, 10, 30}: print(x)
10
20
30
>>> for x in {200, 100, 300}: print(x)
200
100
300
For example you can see that by adding a large power of two to the numbers you can see that they are sorted by the remainder (as it seems that the default number of buckets is a power of two):
> for x in {2048 + 1, 256 + 3, 1024 + 2, 512 + 4}: print(x)
2049
1026
259
516whereas each time you run id() on a number outside that range you will get a different id as a new object is instantiated
>>> for x in 1, 2, 3: ...
But that's just wrong. Tuples are not just immutable lists. They're for heterogeneous collections and are meant to be destructured or otherwise have their elements acted on individually. Lists are for homogeneous collections and they're meant to be looped over. Immutability is neither here nor there. The performance benefit is negligible and irrelevant.
If you don't believe me, look at how type hints work: an arbitrary-length list of integers is typed like `list[int]`, but you have to write an arbitrary-length tuple of integers like `tuple[int, ...]`. It looks different -- why? Because the way you're meant to use tuples is like `tuple[int, int, str, datetime]`, where there's a fixed number of elements and each one has a distinct meaning, where it usually doesn't make sense to loop over and treat them the same.
It's also why there's list comprehension but no tuple comprehension.
The suggestion in the example is just plain incorrect.
I've never read this opinion before, so I don't think it was intended from the get-go or became a commonly adopted standard. I can see some sense to it, though.
But lists aren't just for looping. They often get looped over, but that's true for any collection, even dictionaries.
>It's also why there's list comprehension but no tuple comprehension.
That's because (x for x in y) was chosen for making generators. Just stick tuple in front and you've got a tuple comprehension.
https://docs.python.org/3/library/stdtypes.html
>Tuples are immutable sequences, typically used to store collections of heterogeneous data (such as the 2-tuples produced by the enumerate() built-in). Tuples are also used for cases where an immutable sequence of homogeneous data is needed (such as allowing storage in a set or dict instance).
GvR email from 2003:
https://mail.python.org/pipermail/python-dev/2003-March/0339...
>Tuples are for heterogeneous data, list are for homogeneous data. >Tuples are not read-only lists.
Immutable sequence is a use-case, but it's a secondary use-case. The primary use-case is for record-type data.
>But lists aren't just for looping. They often get looped over, but that's true for any collection, even dictionaries.
My point was more that tuples are not primarily suited for looping over, not that lists exclusively are.
>That's because (x for x in y) was chosen for making generators. Just stick tuple in front and you've got a tuple comprehension.
But list comprehensions were in the language for several years before generators came around. There was a time when [x for x in foo] worked but (x for x in foo) was a syntax error. Why didn't they make it a tuple at the time? Because that's not what tuples are for.
If you look at everywhere tuples show up in the standard library, it's almost always in some context where looping over the collection is not a primary consideration. The clearest example of this dichotomy is in the string methods: the str.split method gives you a list of strings, because the result can be of arbitrary length and you're likely going to loop over it. But str.partition gives a tuple because there are always exactly 3 elements in the result and you're meant to destructure it.
They aren’t just immutable lists logically, but since Python (while it can use a static typechecker for verifying correctness to some extent) doesn’t use type information about what is stored in containers for storage implementation, effectively at runtime they are just immutable lists.
But, even logically, and apart from that implementation consideration, immutable lists are one of the things they are, are intended to be, ans are designed to be used for.
> They're for heterogeneous collections and are meant to be destructured or otherwise have their elements acted on individually.
That is another use case, yes. The existence of that use case does not negate the immutable list use case.
> It looks different -- why?
tuple type hints have a form (that you point out) for homogenous arbitrary length immutable lists as well as one for fixed-length potentially-heterogenous ordered collections because tuples are intended to serve both purposes, not just the latter.
> It's also why there's list comprehension but no tuple comprehension.
No, genexps using () and having broader utility is why there are no tuple comprehensions. Heck, if genexps were around first, we might not have list, set, or dictionary comprehensions, either.
if x in {1, 2, 3}:
print(x)There used to be a difference in python2 -- the list option would turn into n LOAD_CONSTs and a BUILD_LIST, whereas the tuple option would just emit one LOAD_CONST.
import dis
def with_tuple():
for i in (1,2,3):
print(i)
def with_list():
for i in [1,2,3]:
print(i)
print("with_tuple")
dis.dis(with_tuple)
print("with_list")
dis.dis(with_list)
My understanding is that tuples are always more efficient and preferred unless mutability is needed.
with open("foo") as a:
contents = a.read(100)
> main.py:1:1 [FURB101]: Use `y = Path(x).read_text()` instead of `with open(x, ...) as f: y = f.read()`
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.
> 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.
Basically, they are not equivalent.
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.
In the example, that issue could be avoided with minimal cost by reading the file lazily:
with open(filename) as f:
for line in f:
do_same_things_with(line)
However, in general switching to a lazy design won’t always be as safe and easy as it is here, so I’m not sure what I’d ideally want a tool to say in this situation. You’d need much more sophisticated analysis of the original code’s design if you wanted to do something like warning about unnecessarily reading an entire file and suggesting the lazy alternative. It doesn’t seem like a good idea to warn about all uses of the functions that will read the whole file at once, because often with Python scripts that’s a simple and effective way get the required job done and the added complexity from a more scalable design isn’t needed or justified.
Writing good style checkers is hard. :-)
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)
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.
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.
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.
other python implementations, like PyPy, does not.
The only guarantee CPython makes is that it may eventually close the file once all references are gone. The fact that it does it immediately and reliably in this specific case is an implementation detail and should not be relied upon.
Use context managers when things need to be closed and save yourself trouble down the line.
(Path("some/deep/folder").parent / "file.txt").read_text()
Similar to black, Refurb is opinionated. There are probably going to be checks which you will ignore. Somewhat of a non-goal with Refurb is bringing to light some of the different ways of writing Python code, and the pathlib module is, IMHO, a very underutilized part of the stdlib.
May I then assume that Refurb is better suited to work on new code? To get back to open(), I don’t know if I would want to go through my company’s legacy code (which runs on Python 3.10) and start replacing battle-tested code everywhere. Same would go for tuple literals over lists in certain circumstances.
I think this will go really well with choosing an „acceptable“ subset of Python for your project, similar to how some companies choose a subset of C++ and then stick with that throughout a project.
I agree that going and updating your company's codebase would probably be a bad idea, considering that there are some kinks here and there. Refurb tries its best, but it is very early on in its development, so errors that are emitted should be taken as suggestions, not gospel.
...$ ~/.local/bin/refurb -h
refurb: unsupported option "-h"
...$ ~/.local/bin/refurb --help
refurb: unsupported option "--help"
...$ ~/.local/bin/refurb
refurb: no arguments passed
ah. have fun
Will follow the project.
---
[^1]: https://sourcery.ai/
It looks like the checks are in the category of standardizing on modern python syntax features, sort-of one step up from an autoformatter like yapf or black. Is this correct?
Thanks for mentioning that project!
Does it really only run one file at a time? I'd love for a recursive folder option.
refurb file.py folder/ another_folder/
Hope that helps!
while we are talking modern Python, the type hints fever seems to be receding. such a good thing in my opinion. i expected this tool to bark about types but am glad the author didn't go that far.
nice tool.