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

Add python 3 support to cpplint and cpplint_unittest #528

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

m-chaturvedi
Copy link
Contributor

No description provided.

@@ -37,6 +37,7 @@
import os
import random
import re
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.

this is a third party import, so it should be after unittest but before cpplint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@@ -3071,7 +3072,8 @@ def DoTest(self, raw_bytes, has_invalid_utf8):
error_collector = ErrorCollector(self.assert_)
cpplint.ProcessFileData(
'foo.cc', 'cc',
unicode(raw_bytes, 'utf8', 'replace').split('\n'),
six.ensure_text(raw_bytes, encoding='utf8',
errors='replace').split('\n'),
Copy link
Member

Choose a reason for hiding this comment

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

indent is off -- it should align with ( above

that said, you don't need six. just write raw_bytes.decode('utf-8', 'replace').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@@ -3104,7 +3106,7 @@ def testBadCharacters(self):
cpplint.ProcessFileData(
'nul_utf8.cc', 'cc',
['// Copyright 2014 Your Company.',
unicode('\xe9x\0', 'utf8', 'replace'), ''],
six.ensure_text(b'\xe9x\0', encoding='utf8', errors='replace'), ''],
Copy link
Member

Choose a reason for hiding this comment

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

indent is off -- should be aligned with [ above

also as above, you don't need six. call .decode() on the byte directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@@ -5744,9 +5748,10 @@ def testQuietWithErrors(self):

def testNonQuietWithoutErrors(self):
# This will succeed. We filtered out all the known errors for that file.
(return_code, output) = self._runCppLint('--filter=' +
(return_code, output_bytes) = self._runCppLint('--filter=' +
'-legal/copyright,' +
Copy link
Member

Choose a reason for hiding this comment

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

indent is off now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@@ -5758,10 +5763,11 @@ def testNonQuietWithoutErrors(self):

def testQuietWithoutErrors(self):
# This will succeed. We filtered out all the known errors for that file.
(return_code, output) = self._runCppLint('--quiet',
(return_code, output_bytes) = self._runCppLint('--quiet',
'--filter=' +
Copy link
Member

Choose a reason for hiding this comment

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

indent is off now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@m-chaturvedi m-chaturvedi force-pushed the py3_support_google_repo branch from a4bde71 to 9670c3d Compare February 27, 2020 21:28
@m-chaturvedi m-chaturvedi changed the title Add python 3 support to cpplint_unittest Add python 3 support to cpplint and cpplint_unittest Feb 27, 2020
@m-chaturvedi
Copy link
Contributor Author

m-chaturvedi commented Feb 27, 2020

@vapier I was making these changes to the tests for our internal cpplint which already has support for python3.
I realized that I need to change the corresponding cpplint.py here as well.

We need to substitute assertEquals with assertEqual and assert_ with assertTrue to remove the deprecation warnings (will make another PR once / if this is accepted):
https://docs.python.org/3.3/library/unittest.html#deprecated-aliases

@vapier
Copy link
Member

vapier commented Feb 27, 2020

@markmentovai what's the story with cpplint in this repo ? is this the master copy ? or is it in google3 ? or is it cpplint/cpplint ?

@markmentovai
Copy link
Member

cpplint here was maintained by @eglaysher while he was at Google, and hasn’t seen much action since.

I didn’t know anything about cpplint/cpplint until moments ago.

@gpshead
Copy link
Contributor

gpshead commented Feb 28, 2020

I have pointed others who have come along at the https://github.com/cpplint/cpplint community maintained one as well due to lack of traction in getting any of us to maintain the version in this repo.

Googlers: internally our devtools/cpplint/ version has been updated to use Python 3. I believe all that needs is a copybara config setup to export it+test to this repo from time to time. For an example of how to set that up, code search internally for config/pylint_only/copy.bara.sky.

Alternatively, if we're not going to maintain the version here I suggest deleting it and officially pointing external users at the aformentioned cpplint project.

@vapier
Copy link
Member

vapier commented Feb 28, 2020

if we do have an internal copy to publish from, then publishing it here would be helpful. i think i've had it come up with other public repos that we wanted updated cpplint scripts.

@markmentovai
Copy link
Member

It’s just a question of ownership. If there are internal owners for cpplint, it’d be best for them to be involved in the process. But even if we can’t get that, if someone’s willing to do it best-effort (as Elliot had been doing) or even as a one-off, it’d be an improvement.

aaronliu0130 added a commit to cpplint/cpplint that referenced this pull request Feb 4, 2024
See google#528 (comment). Google is no longer maintaining the public version of cpplint.
@vapier vapier added cpplint and removed cla: yes labels Jul 8, 2024
@tkruse
Copy link

tkruse commented Aug 3, 2024

See #837

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

Successfully merging this pull request may close these issues.

7 participants