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

update contrib/python with py3 compat #6184

Merged
merged 2 commits into from Jul 20, 2018

Conversation

Projects
None yet
3 participants
@GoingTharn
Copy link
Contributor

GoingTharn commented Jul 19, 2018

Problem

Upgrading to py3

Only substantive change:
original:
list(map(lambda p:p if p != 'pep8' else 'pycodestyle', plugins))

For loop should have replaced pep8 with pycodestyle.

Solution

This is the same PR as previous only cleaning up the accidental merge w/o rebase.

@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

Thank you John!

@@ -197,7 +198,7 @@ def iter_logical_lines(self, blob):
yield self.translate_logical_line(
line_number_start,
token_start[0] + (1 if token_type is tokenize.NEWLINE else -1),
list(filter(None, contents)),
list([_f for _f in contents if _f]),

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 19, 2018

Contributor

I don't think the call to list is necessary, since this is a list comp

This comment has been minimized.

@GoingTharn

GoingTharn Jul 19, 2018

Contributor

correct

@@ -259,7 +260,7 @@ def flatten_lines(*line_or_line_list):

def __init__(self, code, severity, filename, message, line_range=None, lines=None):
if severity not in self.SEVERITY:
raise ValueError('Severity should be one of {}'.format(' '.join(self.SEVERITY.values())))
raise ValueError('Severity should be one of {}'.format(' '.join(list(self.SEVERITY.values()))))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 19, 2018

Contributor

Likewise I think you don't need list(). The str.format() will call the values when it needs them, avoiding the intermediate call to list().

This comment has been minimized.

@GoingTharn

GoingTharn Jul 19, 2018

Contributor

(checking this)

@@ -48,7 +49,7 @@


def strip_newline(stmt):
return textwrap.dedent('\n'.join(filter(None, stmt.splitlines())))
return textwrap.dedent('\n'.join([_f for _f in stmt.splitlines() if _f]))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 19, 2018

Contributor

I think this can be converted to a generator. Simply remove the [], and we should get minor performance enhancements.

This comment has been minimized.

@GoingTharn

GoingTharn Jul 19, 2018

Contributor

Think we'd need to wrap it in () but correct we don't need the intermediate list

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 19, 2018

Contributor

You shouldn’t need to wrap in (), since there’s nothing else in that expression. You only need to wrap when doing something like next((gen), None)

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 19, 2018

Will merge afterhours.

@stuhood stuhood merged commit 4670779 into pantsbuild:master Jul 20, 2018

1 check passed

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

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

update contrib/python with py3 compat (pantsbuild#6184)
### Problem
Upgrading to py3

Only substantive change: 
original:
            list(map(lambda p:p if p != 'pep8' else 'pycodestyle', plugins))

For loop should have replaced pep8 with pycodestyle.

### Solution

This is the same PR as previous only cleaning up the accidental merge w/o rebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment