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

[BEAM-1251] Modernize Python 2 code to get ready for Python 3 #5842

Merged

Conversation

cclauss
Copy link

@cclauss cclauss commented Jun 30, 2018

Signed-off-by: cclauss cclauss@bluewin.ch

Please add a meaningful description for your change here

Fix Python 3 syntax errors and undefined names.

@aaltay I am confused why it is necessary to keep making these PRs. I thought that we had put in place automated testing on Python 3 that would use flake8 to flag syntax errors and undefined names so that these anachronisms would not be checked into the codebase.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
--- --- --- ---

@pabloem
Copy link
Member

pabloem commented Jul 2, 2018

It seems that Lint and Docs failed. You can run them via tox -e py27-lint, tox -e py3-lint and tox -e docs from within sdks/python/

@charlesccychen
Copy link
Contributor

Note that this change conflicts with #5373, which fixes some of the same issues.

@cclauss
Copy link
Author

cclauss commented Jul 2, 2018

@charlesccychen It is not so much that these PRs are conflicting. It is more like they are trying to make many of the same changes. We have been trying for a while to make this repo Python 3 compatible by running futurize so let's not slow things down by waiting. Full speed ahead on all efforts.

@cclauss
Copy link
Author

cclauss commented Jul 3, 2018

@superbobry @holdenk Could I please get your review on these changes?

Copy link
Contributor

@holdenk holdenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending Jenkins.

@pabloem
Copy link
Member

pabloem commented Jul 3, 2018

LGTM. Can you rebase the change?

@cclauss cclauss force-pushed the print_is_a_function_in_python3 branch 2 times, most recently from b826a83 to f674206 Compare July 4, 2018 04:17
Signed-off-by: cclauss <cclauss@bluewin.ch>
@cclauss cclauss force-pushed the print_is_a_function_in_python3 branch from f674206 to 38a317a Compare July 4, 2018 04:24
@cclauss
Copy link
Author

cclauss commented Jul 4, 2018

Rebased

@charlesccychen
Copy link
Contributor

Thanks @cclauss!

@charlesccychen charlesccychen merged commit 997ee3a into apache:master Jul 8, 2018
@cclauss cclauss deleted the print_is_a_function_in_python3 branch July 8, 2018 21:49
@@ -29,12 +29,18 @@
'sudo apt-get install python-beautifulsoup4'.

"""
from __future__ import print_function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The website source code is currently being migrated from https://github.com/apache/beam-site, but is not yet ready. Website changes in apache/beam will be overwritten on next merge. Please contribute changes at apache/beam-site according to the website contribution guide. You can track migration progress via [BEAM-4493].

Do you know if this change was also made in the apache/beam-site repository? If not, it will need to be migrated.

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

Successfully merging this pull request may close these issues.

None yet

5 participants