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] Change unicode --> six.text_type for Python 3 #4697

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

cclauss
Copy link

@cclauss cclauss commented Feb 16, 2018

unicode was removed in Python 3 because all str are Unicode so this PR:

@cclauss cclauss changed the title Unicode to six.text type Change unicode --> six.text_type for Python 3 Feb 17, 2018
@pabloem
Copy link
Member

pabloem commented Feb 21, 2018

Python Precommits are broken for now (fix incoming). As advice, it'd be good for you to add a reviewer for your PR. I'll take a look in the next few days.

@cclauss
Copy link
Author

cclauss commented Feb 21, 2018

How do I "add a reviewer"?

@pabloem
Copy link
Member

pabloem commented Feb 21, 2018

There was some breakage in the Python SDK earlier. Could you rebase to master branch?

@pabloem
Copy link
Member

pabloem commented Feb 21, 2018

Jenkins retest this please

from apache_beam.transforms.display import DisplayDataItem
from apache_beam.transforms.display import HasDisplayData
from apache_beam.transforms.display import (DisplayData, DisplayDataItem,
HasDisplayData)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Lint is complaining about these:

 import apache_beam as beam
 from apache_beam.options.pipeline_options import PipelineOptions
-from apache_beam.transforms.display import (DisplayData, DisplayDataItem,
-                                            HasDisplayData)
+from apache_beam.transforms.display import DisplayData
+from apache_beam.transforms.display import DisplayDataItem
+from apache_beam.transforms.display import HasDisplayData

@@ -22,14 +22,15 @@
import unittest
from datetime import datetime

import six

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Lint is complaining of the ordering of this import:

+import hamcrest as hc
 import six
-
-import hamcrest as hc
 from hamcrest.core.base_matcher import BaseMatcher

Copy link
Author

Choose a reason for hiding this comment

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

This one complains when the hamcrest imports are split but also complains when they are put together. The desired order is also different than iSort. A bit difficult to find the correct order.

@cclauss
Copy link
Author

cclauss commented Feb 22, 2018

So now it is saying:

 import hamcrest as hc
+import six
 from hamcrest.core.base_matcher import BaseMatcher
-
-import six

But when I move import six up in between the two hamcrest imports, it will complain that they can not be split. Trying that now...

@cclauss
Copy link
Author

cclauss commented Feb 22, 2018

Now...

Running pylint for module apache_beam:
************* Module apache_beam.transforms.display_test
C: 27, 0: Imports from package hamcrest are not grouped (ungrouped-imports)

@cclauss
Copy link
Author

cclauss commented Feb 24, 2018

Hopefully the # pylint: disable=ungrouped-imports and # pylint: enable=ungrouped-imports trick that I learned from @luke-zhu should placate pylint and this PR should be good to go.

@luke-zhu
Copy link
Contributor

Glad that worked. I've added https://issues.apache.org/jira/browse/BEAM-3745 to track this.

@cclauss
Copy link
Author

cclauss commented Feb 24, 2018

Can someone please advise how I can fix the failing test?

@luke-zhu
Copy link
Contributor

Looks like the problem is unrelated. It's on the java branch and I saw that a different build on the same computer (beam 5) failed similarly here: https://builds.apache.org/job/beam_PreCommit_Java_GradleBuild/2529/console

You could also type in "retest this please" to the the checks again

@cclauss cclauss force-pushed the unicode-to-six.text_type branch 3 times, most recently from 247bdce to 42c125a Compare March 2, 2018 09:24
@cclauss cclauss changed the title Change unicode --> six.text_type for Python 3 [BEAM-1251] Change unicode --> six.text_type for Python 3 Mar 5, 2018
@cclauss
Copy link
Author

cclauss commented Mar 6, 2018

@aaltay Your review please?

@aaltay
Copy link
Member

aaltay commented Mar 6, 2018

@cclauss LGTM. I will merge. As a side question, is it possible for us to update linter rules to avoid regressing?

How do I "add a reviewer"?
You could either tag someone as R: @altay, or add them as a reviewer in the UI on the top right.

@aaltay aaltay merged commit 5b99fe2 into apache:master Mar 6, 2018
@cclauss cclauss deleted the unicode-to-six.text_type branch March 6, 2018 17:32
@cclauss
Copy link
Author

cclauss commented Mar 6, 2018

On update linter rules to keep from backsliding see #4798

On the reviewer tips, they only work if you have the commit bit which I do not have and do not need.

@aaltay
Copy link
Member

aaltay commented Mar 6, 2018

On update linter rules to keep from backsliding see #4798

Thank you, will review it there.

On the reviewer tips, they only work if you have the commit bit which I do not have and do not need

You can add R; .. as a comment to the PR. Beam committer typically look for that tagging. You do not need to be a committer to do that.

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