facebookincubator / Bowler

Safe code refactoring for modern Python.
https://pybowler.io/
MIT License
1.53k stars 89 forks source link

Can Bowler rename the variable in the format string literals? #87

Open sangwoo-joh opened 5 years ago

sangwoo-joh commented 5 years ago

Hi. I have some code that uses format string literals and .format() like below:

def foo(bar, zar):
    print(f'bar is {bar}, not {zar}')
    print('bar is {bar}, not {zar}'.format(bar=bar, zar=zar))

I want to rename the variable name bar to bar_new so the rename.py script is ready:

from bowler import Query

Query('.').select_var('bar').rename('bar_new').idiff()

I hope Bowler recognize the placeholder in string literal, {bar}, but it wasn't:

--- ./format_string.py
+++ ./format_string.py
@@ -1,3 +1,3 @@
-def foo(bar, zar):
+def foo(bar_new, zar):
   print(f'bar is {bar}, not {zar}')
-  print('bar is {bar}, not {zar}'.format(bar=bar, zar=zar))
+  print('bar is {bar}, not {zar}'.format(bar_new=bar_new, zar=zar))
Apply this hunk [y,N,q,a,d,?]? 

I want Bowler to rename

and this is what exactly the Refactor → Rename function does in PyCharm.

Is there a way to refactor my code like this? Thank you for your great tool.

thatch commented 5 years ago

The first part -- renaming the variable within an f-string -- is not currently supported, but I would be glad to review a PR if you feel like adding it. @jreese is that second part a bug, renaming the kwarg in a call? The docs are unclear at https://pybowler.io/docs/api-selectors#select-var

amyreese commented 5 years ago

That looks like a side effect of the way the select_var selector is written, because it basically captures any name element in the tree, which in this case also matches the kwarg. I'm not sure I would call it a "bug" per se, but potentially unexpected behavior in most cases.

As for modifying contents of string literals, we would need to add support for parsing all string literals for f-string-like syntax, which itself would require more than what lib2to3 can offer. I believe something like typed_ast is capable of doing this, but I'm not certain, and at the very least would require a fair amount of work to fit this second method of parsing and transformations into the Bowler workflow.

sangwoo-joh commented 5 years ago

@thatch , @jreese Thanks for the answer! I agree that Bowler need to parse all f-string-like literals, as @jreese said. After I understand the core logic of Bowler itself, I hope I can contribute. Thank you.

sangwoo-joh commented 4 years ago

I found this issue about "f-strings are parsed just as regular strings" on python bug tracker, and it is not resolved yet. Since the mechanism of Bowler's select depends on how lib2to3 parse code, I think this issue should be resolved first... so that we can add a f-string related patterns in select_var.

thatch commented 4 years ago

We could either hack around this in Bowler while we use fissix (lib2to3) or wait until Bowler2 uses LibCST, at which point we can have first-party support for using them. I'm inclined to wait, mainly because I've tried to upstream things to lib2to3 before and it didn't go well.

sangwoo-joh commented 4 years ago

@thatch Thanks for your update. I agree with you that we have to wait. And I'm very excited to hear about the Bowler2 using LibCST!! :heart_eyes: Always thank you for the great tool.

Omar-Elrefaei commented 3 years ago

Any progress or plans on Bowler2.

@sangwoo-joh Did you manage to rename vars in fstrings some other way?

sangwoo-joh commented 3 years ago

@Omar-Elrefaei I've just solved my issue using libCST. :sweat_smile:

Omar-Elrefaei commented 3 years ago

@Omar-Elrefaei I've just solved my issue using libCST. sweat_smile

If you have shareable code that renames variables, I would be in your debt 😳.

sangwoo-joh commented 3 years ago

@Omar-Elrefaei It's a rough example and maybe not suitable for your case, but you can find a sample from my gist: https://gist.github.com/sangwoo-joh/26e9007ebc2de256b0b3deeda051772d

It is true that libcst can parse the f-strings, but it has a tricky thing too: it does not provide APIs to decide whether to visit the child attributes or not; only for child nodes. Because of this lack, recklessly using leave_Name to transform the original code would rename all the Name nodes of the keyword attributes in normal format string (not f-string) such as "some-string".format(keyword=value). I don't want this, so as my gist code does, just tracking each change of the keyword attribute and restoring it after leaving was enough for my issue. But I don't know your case, so be careful about it. I hope it would help you.

Omar-Elrefaei commented 3 years ago

@sangwoo-joh Appreciate it. ❤️