Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dict.items() not converted to list(dict.items()) when iterated over #176

Open
hniksic opened this issue Feb 19, 2019 · 4 comments · May be fixed by #262
Open

dict.items() not converted to list(dict.items()) when iterated over #176

hniksic opened this issue Feb 19, 2019 · 4 comments · May be fixed by #262

Comments

@hniksic
Copy link

hniksic commented Feb 19, 2019

It appears that python-modernize doesn't convert for k, v in d.items() to for k, v in list(d.items()), assuming that the use of items() was an accident in Python 2. This is not always the case - consider the following two functions:

def foo1(d):
    return d.items()

def foo2(d):
    for k, v in d.items():
        if v % 2 == 0:
            del d[k]

When python-modernize is run on the code, it correctly modifies foo1 to return list(d.items()), but it doesn't modify foo2 to iterate over list(d.items()). In our code base this led to many subtle bugs because our Python 2 code was very careful to only use items() (and keys() and values()) because the dictionary was being modified.

Is there a way to opt out of the special-handling of items inside for?
@takluyver
Copy link
Contributor

There isn't a command-line option or anything, but it looks like modernize is modifying the handling from 2to3 slightly to assume for k, v in d.items() is a mistake, rather than deliberate. So you could just comment out the modification and use the logic from 2to3:

https://github.com/python-modernize/python-modernize/blob/a234ce4e185cf77a55632888f1811d83b4ad9ef2/libmodernize/fixes/fix_dict_six.py#L28-L32

@hniksic
Copy link
Author

hniksic commented Feb 19, 2019

@takluyver Thanks for the help. I saw this part of the source, but my question (and this issue) is about enabling this without modifying the source, for the sake of others who encounter this issue.

I would also argue that the default is incorrect and should be changed because it breaks correct code, as shown in the issue description. But this might be a matter of opinion, and explicit opt-out would certainly work for us.

@takluyver
Copy link
Contributor

modernize generally doesn't go in for a lot of options - what you can do is mostly enable and disable specific fixers. You could always disable this one and run the one from 2to3, or one you've customised.

Thinking about it, there was a push a while back to make modernize idempotent, so running it on code you had already modernize-d wouldn't produce further changes. I guess this override was part of that, because without it for k in d.iterkeys() -> for k in d.keys() -> for k in list(d.keys()).

@hniksic
Copy link
Author

hniksic commented Feb 19, 2019

@takluyver The idempotence argument would also require return d.keys() not to be transformed to return list(d.keys()). I am aware that I could work around the problem by not using the fixer in question and switching t 2to3 for that particular fix, but I would prefer an improvement python-modernize correctly, as we otherwise find it a huge help with migration to Python 3.

@yonran yonran linked a pull request Oct 11, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add back list() when iterating over d.items() in a for statement yonran/modernize
2 participants
@takluyver @hniksic