[BEAM-1251] Fix basestring, file(), reduce(), and xrange() for Python 3 #4804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
R: @aaltay |
cclauss
force-pushed
the
basestring-to-six.string_types
branch
2 times, most recently
from
aa74f38
to
1bb620f
Compare
aaltay
reviewed
Mar 8, 2018
| @@ -44,6 +44,7 @@ | |||
| from apache_beam.transforms.window import TimestampCombiner | |||
| from apache_beam.transforms.window import TimestampedValue | |||
| from apache_beam.utils import windowed_value | |||
| from six.moves import xrange | |||
There was a problem hiding this comment.
Can we use range in this file instead? I only see one usage of xrange and it is for list size limited by length of pcollections that is going into a cogroupbykey. Assuming that it will not be a large number, I do not think we need to xrange there.
| @@ -39,6 +39,7 @@ | |||
| from apache_beam.testing.util import assert_that | |||
| from apache_beam.testing.util import equal_to | |||
| from apache_beam.utils.windowed_value import WindowedValue | |||
| from six.moves import reduce | |||
There was a problem hiding this comment.
Should we use functools.reduce instead? The reduce is used below in a snippet and we publish those on our website. The example using reduce directly will be broken for python 3 users (they do not see the imports, only the snippet). However if we replace the usage of reduce with functools.reduce it will be a complete snippet.
cclauss
force-pushed
the
basestring-to-six.string_types
branch
from
1bb620f
to
e35dc23
Compare
|
Done. |
|
LGTM, Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
basestring was removed in Python 3 because all str are Unicode so this PR:
R: @aaltay
DESCRIPTION HERE
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.