-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-3998] Futurize examples subpackage #5652
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
Conversation
|
Thank you. I will wait for @tvalentyn to make the first pass. |
tvalentyn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @Fematich . Overall LGTM, a few minor comments below.
| 'team': row[1], | ||
| 'score': int(row[2]), | ||
| 'timestamp': int(row[3]) / 1000.0, | ||
| 'timestamp': int(row[3]) // 1000.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reguilar division given the float number.
| 'team': row[1], | ||
| 'score': int(row[2]), | ||
| 'timestamp': int(row[3]) / 1000.0, | ||
| 'timestamp': (int(row[3])//1000.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Also not sure why the tool added parenthesis on both sides.
| 'team': row[1], | ||
| 'score': int(row[2]), | ||
| 'timestamp': int(row[3]) / 1000.0, | ||
| 'timestamp': (int(row[3])//1000.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| 'team': row[1], | ||
| 'score': int(row[2]), | ||
| 'timestamp': int(row[3]) / 1000.0, | ||
| 'timestamp': (int(row[3])//1000.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| googledatastore.helper.add_key_path(entity.key, kind, str(uuid.uuid4())) | ||
| googledatastore.helper.add_properties(entity, | ||
| {'content': six.text_type(content)}) | ||
| {'content': unicode(content)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several other modules in the examples package also use six, let's clean them up.
See: https://github.com/apache/beam/search?q=six.text_type&unscoped_q=six.text_type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all the remaining six imports with the latest commit.
|
Thanks, @Fematich. Did you push the updated commit? I still see |
|
Sorry for that! Apparently I didn't push my latest commit, fixed now |
charlesccychen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Fematich. This LGTM.
[BEAM-3998] Futurize examples subpackage
[BEAM-3998] Futurize examples subpackage
This pull request prepares the examples subpackage for Python 3 support. This PR is part of a series in which all subpackages will be updated using the same approach.
This approach has been documented here and the first pull request in the series (Futurize coders subpackage) demonstrating this approach can be found at #5053.
R: @aaltay @tvalentyn @RobbeSneyders