Message327690
Thanks for the patch but I feel adding type checks will add more complexity to the code being converted and there are a lot of cases to handle. I have some code below where running 2to3 will just wrap the keys() call to a list but with the patch the if call might be have an inner if call if I am understanding the patch correctly as you have handled the for loop case.
$ cat ../backups/bpo34978.py
a = {1: 1}
True if 1 in a.keys() else False
$ 2to3 -w ../backups/bpo34978.py
RefactoringTool: Skipping optional fixer: buffer
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: set_literal
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Refactored ../backups/bpo34978.py
--- ../backups/bpo34978.py (original)
+++ ../backups/bpo34978.py (refactored)
@@ -1,3 +1,3 @@
a = {1: 1}
-True if 1 in a.keys() else False
+True if 1 in list(a.keys()) else False
RefactoringTool: Files that need to be modified:
RefactoringTool: ../backups/bpo34978.py
➜ cpython git:(master) cat ../backups/bpo34978.py
a = {1: 1}
True if 1 in a.keys() else False
With the patch the above might be as below which throws syntax error.
True if 1 in list(a.keys()) if type(a) == dict else a.keys() else False
I think this is one case which can be handled like for but there might be other places where this might return invalid results that I couldn't think of like nested if clauses in list comprehensions and so on. I think this fixer was written with the notion that .keys() and .values() is a very common method for dict and these methods are not something many people define themselves as far as I have seen from other's code since they are more attached with dict so that the fixer affects those cases. I think the gain is minimal here with cases to handle.
`list(d.keys())` seems to be more Pythonic than `list(d.keys) if type(d) == dict else d.keys()` though it provides some additional type checks.
# With current 2to3
keys = list(a.keys())
if 1 in list(a.keys()):
True
else:
False
# With patch this also increases line length and more to understand when looking at the code
keys = list(a.keys()) if type(a) == dict else a.keys()
if 1 in list(a.keys()) if type(a) == dict else a.keys():
True
else:
False
I couldn't find any discussions at the moment to see if this was discussed earlier when 2to3 was written. This is just my suggestion and I will leave it to Benjamin and others for thoughts on this. Feel free to correct me if I am misunderstanding the patch where it was handled.
Thanks |
|
Date |
User |
Action |
Args |
2018-10-14 05:38:06 | xtreak | set | recipients:
+ xtreak, benjamin.peterson, devarakondapranav |
2018-10-14 05:38:06 | xtreak | set | messageid: <1539495486.31.0.788709270274.issue34978@psf.upfronthosting.co.za> |
2018-10-14 05:38:06 | xtreak | link | issue34978 messages |
2018-10-14 05:38:05 | xtreak | create | |
|