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

tools: bump cpplint.py to 3d8f6f876d #25771

Merged
merged 2 commits into from
Jan 30, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Jan 28, 2019

  • Preserve 3 node-core checks
  • Remove TAP to logfile (unused)

Fixes: #25760
Refs: https://github.com/cpplint/cpplint/blob/3d8f6f876dd6e3918e5641483298dbc82e65f358/cpplint.py

/CC @nodejs/build-files @nodejs/python

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jan 28, 2019
@refack refack self-assigned this Jan 28, 2019
@refack refack added c++ Issues and PRs that require attention from people who are familiar with C++. python PRs and issues that require attention from people who are familiar with Python. labels Jan 28, 2019
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

Rubber Stamp LGTM

@cclauss cclauss mentioned this pull request Jan 28, 2019
4 tasks
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2019
@@ -1289,8 +1297,8 @@ def RepositoryName(self):
"""
fullname = self.FullName()
# XXX(bnoordhuis) Expects that cpplint.py lives in the tools/ directory.
toplevel = os.path.abspath(os.path.join(os.path.dirname(__file__), '..')) \
.replace('\\', '/').decode('utf-8')
toplevel = os.path.abspath(os.path.join(os.path.dirname(__file__), '..')).replace('\\', '/')
Copy link
Member

Choose a reason for hiding this comment

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

Long line.

@@ -4677,7 +4677,7 @@ def _ClassifyInclude(fileinfo, include, is_system):

# Headers with C++ extensions shouldn't be considered C system headers
if is_system and os.path.splitext(include)[1] in ['.hpp', '.hxx', '.h++']:
is_system = False
is_system = False
Copy link
Member

Choose a reason for hiding this comment

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

Indent error (or does this come from upstream?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Google favors is 2 space indentation. There is a pretty good chance that this code was formatted with https://github.com/google/yapf

Copy link
Contributor Author

@refack refack Jan 28, 2019

Choose a reason for hiding this comment

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

All white-space changes are lifted verbatim from https://github.com/cpplint/cpplint/blob/3d8f6f876dd6e3918e5641483298dbc82e65f358/cpplint.py

P.S. I'll refactor the commits to make that easier to review

@refack
Copy link
Contributor Author

refack commented Jan 30, 2019

PR-URL: nodejs#25771
Fixes: nodejs#25760
Refs: https://github.com/cpplint/cpplint/blob/3d8f6f876dd6e3918e5641483298dbc82e65f358/cpplint.py
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
* Preserve 3 node-core checks
* Preserve patch to `FileInfo.RepositoryName`
* Remove TAP to logfile (unused)

PR-URL: nodejs#25771
Fixes: nodejs#25760
Refs: https://github.com/cpplint/cpplint/blob/3d8f6f876dd6e3918e5641483298dbc82e65f358/cpplint.py
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2019
@refack refack merged commit 4d19300 into nodejs:master Jan 30, 2019
@refack refack deleted the bump-cpplint-3d8f6f876d branch January 30, 2019 23:28
addaleax pushed a commit that referenced this pull request Feb 1, 2019
PR-URL: #25771
Fixes: #25760
Refs: https://github.com/cpplint/cpplint/blob/3d8f6f876dd6e3918e5641483298dbc82e65f358/cpplint.py
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit that referenced this pull request Feb 1, 2019
* Preserve 3 node-core checks
* Preserve patch to `FileInfo.RepositoryName`
* Remove TAP to logfile (unused)

PR-URL: #25771
Fixes: #25760
Refs: https://github.com/cpplint/cpplint/blob/3d8f6f876dd6e3918e5641483298dbc82e65f358/cpplint.py
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos mentioned this pull request Feb 14, 2019
@refack refack removed their assignment Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tools: need to upgrade tools/cpp_lint.py to be Python 3 compatible
6 participants