Karthikeyan Singaravelan <[email protected]> added the comment:
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
----------
nosy: +xtreak
_______________________________________
Python tracker <[email protected]>
<https://bugs.python.org/issue34978>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
