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.

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

fix_range is not idempotent #52

Closed
brettcannon opened this issue Sep 6, 2014 · 7 comments · Fixed by #60
Closed

fix_range is not idempotent #52

brettcannon opened this issue Sep 6, 2014 · 7 comments · Fixed by #60

Comments

@brettcannon
Copy link
Contributor

Now that libmodernize.fixes.fix_range is being used instead of lib2to3.fixes.fix_xrange, there is the problem that the fixer is no longer idempotent, i.e. list(range(1)) will become list(list(range(1))) and from six.moves import range; range(1) becomes from six.moves import range; list(range(1)).

Possible solutions are:

  1. Do the necessary bookkeeping for both range and xrange (I suspect range is doable with some not rule in the pattern, but xrange might be a pain)
  2. Only fix xrange and assume anyone using range to make a list can do the fix themselves
  3. Go back to using lib2to3.fixes.fix_xrange which turns xrange into range
@brettcannon
Copy link
Contributor Author

I think this might be resolved by turning back on lib2to3.fixes.fix_xrange and then having fix_range just do from six.moves import range whenever range() or xrange() is detected (which will let lib2to3 handle the proper translation of the functions themselves). That fixer is smart enough to be idempotent and do the smartest thing based on context and helps keep our fixer simple.

@takluyver
Copy link
Contributor

That sounds elegant. I'm thinking through it looking for edge cases.

@takluyver
Copy link
Contributor

I think the 2to3 fixer is not quite idempotent. Consider:

a = xrange(1, 20)
# Converted to
a = range(1, 20)
# Converted to
a = list(range(1,20))

This only affects an xrange in a non-iterator context, and it only has one extra step before it becomes stable. If we want it to be properly idempotent, we can subclass/copy the fixer and make it skipped if six.moves.range is imported.

@daira
Copy link
Contributor

daira commented Sep 9, 2014

I think we should do what @takluyver suggests (subclass the lib2to3.fixes.fix_xrange fixer and make it skipped if six.moves.range is imported).

@daira
Copy link
Contributor

daira commented Sep 9, 2014

BTW we should call the subclass fixer fix_range_six.

@daira
Copy link
Contributor

daira commented Sep 9, 2014

Re: "it only has one extra step before it becomes stable" -- yes, but that step is incorrect: if the call was originally to xrange, it shouldn't be transformed to list(range(...)), since that will cause a silent increase in memory usage for large ranges, as per #23.

@daira daira added this to the Release 0.3.1 milestone Sep 9, 2014
@brettcannon
Copy link
Contributor Author

+1 for the subclass. This will also be a good test of the approach for things like fix_dict so we can leverage lib2to3's dict fixer for dict.items() but do our own six approach for dict.iteritems().

brettcannon added a commit to brettcannon/python-modernize that referenced this issue Sep 12, 2014
Along the way, move the fixer over to `lib2to3.fixes.fix_xrange` to
piggyback on all the work done there.

Closes PyCQA#52 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants