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-1762] Make error message python 3.6 compatible #2275

Closed
wants to merge 3 commits into from

Conversation

jrmlhermitte
Copy link

No description provided.

@aviemzur
Copy link
Member

I think the JIRA ticket referred to in this PR's title is the wrong one.

@jrmlhermitte jrmlhermitte changed the title [BEAM-1672] Make error message python 3.6 compatible [BEAM-1762] Make error message python 3.6 compatible Mar 20, 2017
@jrmlhermitte
Copy link
Author

@aviemzur thank you

@aaltay
Copy link
Member

aaltay commented Mar 20, 2017

R: @tibkiss

@tibkiss
Copy link
Contributor

tibkiss commented Mar 20, 2017

Nice catch -- I was not aware of the deprecation of the old string formatting.

I only see one problem with 'str.format': It is only supported from 2.6 onwards.
If we want to be compatible with 2.x and 3.x then we could use string concatenation:
'It is not supported on Python' + str(sys.version_info)

@jrmlhermitte
Copy link
Author

that's a good idea, thanks for the feedback. I added it.

@tibkiss
Copy link
Contributor

tibkiss commented Mar 20, 2017

LGTM. Thanks for the fix, @ordirules!

@asfbot
Copy link

asfbot commented Mar 20, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8586/
--none--

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.131% when pulling 7139a91 on ordirules:errormsg into 6fa1fca on apache:master.

@asfbot
Copy link

asfbot commented Mar 20, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8585/
--none--

@aaltay
Copy link
Member

aaltay commented Mar 20, 2017

Jenkins: retest this please

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 70.121% when pulling 7139a91 on ordirules:errormsg into 6fa1fca on apache:master.

@asfbot
Copy link

asfbot commented Mar 20, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8587/
--none--

@@ -69,7 +69,7 @@
if not (sys.version_info[0] == 2 and sys.version_info[1] == 7):
raise RuntimeError(
'Dataflow SDK for Python is supported only on Python 2.7. '
'It is not supported on Python [%s].' % sys.version_info)
'It is not supported on Python '+ str(sys.version_info) + '.')
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to put [] around the version or remove the . at the end to avoid confusion?

Copy link
Author

Choose a reason for hiding this comment

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

oops yes I accidentally remove those. changed, thanks!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.128% when pulling 6993ea1 on ordirules:errormsg into 6fa1fca on apache:master.

@asfbot
Copy link

asfbot commented Mar 20, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8594/
--none--

@aaltay
Copy link
Member

aaltay commented Mar 20, 2017

LGTM. Thank you @ordirules

@asfgit asfgit closed this in 8d24098 Mar 20, 2017
@jrmlhermitte
Copy link
Author

@aaltay no problem.
I have a question actually. Is there any work for python 3 support in the future?
I can help contribute if necessary. I couldn't find anything about this anywhere, including stackoverflow.
thanks!

@tibkiss
Copy link
Contributor

tibkiss commented Mar 29, 2017

@ordirules : Sorry for coming back on your question delayed.

There are multiple JIRAs open regarding 2 to 3 conversion:
https://issues.apache.org/jira/browse/BEAM-1373
https://issues.apache.org/jira/browse/BEAM-1251

As @chamikaramj points out in BEAM-1373 it is not a trivial task due to the string array / unicode conversion. I'd also like to mention that we don't have good UT coverage everywhere in the Python-SDK, it might make sense to extends the UTs where the coverage is low (<50%). Tracking JIRA for that effort is BEAM-1680.

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.

6 participants