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

Python 3 fixes - fix base folder #6252

Merged
merged 9 commits into from Jul 28, 2018

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Jul 27, 2018

Everything should be green with Py2 and Py3, excluding the native engine issue discussed on Slack + pending PRs #6251, #6245, and #6244.

@@ -16,6 +18,7 @@ def hash_all(strs, digest=None):
"""
digest = digest or hashlib.sha1()
for s in strs:
s = ensure_binary(s)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 27, 2018

Contributor

Hasher requires byte strings. Most of the changes made below are converting to bytes.

@@ -93,6 +93,18 @@ def components(self):
def _is_valid_operand(self, other):
return hasattr(other, '_components')

def _fill_value_if_missing(self, ours, theirs):

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 27, 2018

Contributor

Python 2 allows comparing ints and strs, but Py3 doesn't. So, the old fillvalue of 0 for missing values would not work. Instead, it should be 0 if dealing with ints and '' if dealing with strs.

__lt__ was changed to have a fillvalue of None, which is then used in this helper function to determine if it should be a str or int value.

@@ -93,6 +93,18 @@ def components(self):
def _is_valid_operand(self, other):
return hasattr(other, '_components')

def _fill_value_if_missing(self, ours, theirs):
if theirs is None:
return ours, type(ours)()

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 27, 2018

Contributor

type(v)() is a way to get the zero value for that type. It's equivalent to, for example, calling str().

I played around with a couple other implementations for this function, including a dictionary that associates the type with corresponding default value. This seemed to be the cleanest and most generic approach, but I do recognize type(ours)() may look cryptic. Happy to add a comment or go back to that dictionary implementation if you all prefer.

This comment has been minimized.

@stuhood

stuhood Jul 28, 2018

Member

A comment would be good. I've never seen this pattern, and would be confused by it.

@stuhood
Copy link
Member

stuhood left a comment

Thanks.

@@ -93,6 +93,18 @@ def components(self):
def _is_valid_operand(self, other):
return hasattr(other, '_components')

def _fill_value_if_missing(self, ours, theirs):
if theirs is None:
return ours, type(ours)()

This comment has been minimized.

@stuhood

stuhood Jul 28, 2018

Member

A comment would be good. I've never seen this pattern, and would be confused by it.

@stuhood stuhood merged commit 38ecc93 into pantsbuild:master Jul 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-fixes_fix-base branch Jul 30, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Python 3 fixes - fix base folder (pantsbuild#6252)
Everything should be green with Py2 and Py3, excluding the native engine issue discussed on Slack + pending PRs pantsbuild#6251, pantsbuild#6245, and pantsbuild#6244.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment