Author xtreak
Recipients benjamin.peterson, devarakondapranav, xtreak
Date 2018-10-14.05:38:05
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1539495486.31.0.788709270274.issue34978@psf.upfronthosting.co.za>
In-reply-to
Content
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
History
Date User Action Args
2018-10-14 05:38:06xtreaksetrecipients: + xtreak, benjamin.peterson, devarakondapranav
2018-10-14 05:38:06xtreaksetmessageid: <1539495486.31.0.788709270274.issue34978@psf.upfronthosting.co.za>
2018-10-14 05:38:06xtreaklinkissue34978 messages
2018-10-14 05:38:05xtreakcreate