Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott commented Oct 6, 2020

  • Remove unnecessary setting of classinfo where it is set again before
    it is used.
  • Change whitelisted_functions to allowed_functions.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

* Remove unnecessary setting of classinfo where it is set again before it is used. * Change whitelisted_functions to allowed_functions.
@nodejs-github-botnodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 6, 2020
@richardlau
Copy link
Member

Are we ever likely to sync this with upstream? The last time we did was #27098 and it looks like whitelisted_functions is gone from https://github.com/cpplint/cpplint via cpplint/cpplint@08268ef / cpplint/cpplint@64de843.

@Trott
Copy link
MemberAuthor

Trott commented Oct 6, 2020

Are we ever likely to sync this with upstream? The last time we did was #27098 and it looks like whitelisted_functions is gone from https://github.com/cpplint/cpplint via cpplint/cpplint@08268ef / cpplint/cpplint@64de843.

It's also gone from https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py, if we want to sync from that upstream instead. (I don't know the relationship between the two other than that one is a fork of the other on GitHub.)

@Trott
Copy link
MemberAuthor

Trott commented Oct 6, 2020

The superfluous classinfo assignment still appears in the upstream fork, but I'm happy to submit the PR for that there if that's the better way to go.

@richardlau
Copy link
Member

Are we ever likely to sync this with upstream? The last time we did was #27098 and it looks like whitelisted_functions is gone from https://github.com/cpplint/cpplint via cpplint/cpplint@08268ef / cpplint/cpplint@64de843.

It's also gone from https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py, if we want to sync from that upstream instead. (I don't know the relationship between the two other than that one is a fork of the other on GitHub.)

Me neither, but whatever we decide to sync from I'd like to keep the number of floating patches we have to maintain to a minimum.

@cclauss
Copy link
Contributor

cclauss commented Oct 6, 2020

My vote would be https://github.com/cpplint/cpplint -- I have made contributions there in the past and it has gone quite smoothly. It is 300+ commits ahead of the other and only 7 commits behind.

@Trott
Copy link
MemberAuthor

Trott commented Oct 9, 2020

This will probably be supplanted by #35569 and a follow-on PR to that, so I'll move this to draft for now, and close it when the relevant changes have landed to make this obsolete.

@TrottTrott marked this pull request as draft October 9, 2020 13:06
@TrottTrott closed this Dec 11, 2020
@TrottTrott deleted the cpplint-classinfo branch December 11, 2020 13:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Trott@richardlau@cclauss@addaleax@watilde@nodejs-github-bot