How to lint c++ code

[gdams]

Okay so I have thrown multiple C++ linting solutions around but can't seem to please everyone so I am going to go ahead and summarize them here:

Solution 1 - Include cpplint.py

Pros:

• Can be used offline
• Can edit the rules to suit the module

Cons:

• Massive file (6000 lines)
• Needs to be updated manually to remain consistent with node core

Solution 2 - Use an npm module which contains the cpplint.py file

Pros:

• No massive file unless required
• Can be used offline
• Could potentially be adopted by node core
• Can be shared with out nodejs modules

Cons:

• Someone still needs to manage it (if node core doesn't want to adopt it then we have two files that can become out of sync)

Solution 3 - Use a Makefile which curls cpplint.py from node core

Pros:

• No massive file unless required
• No need to maintain the file as node core already does this

Cons:

• Using a Makefile has compatibility issues with windows/aix
• Can't be used offline

Would love to hear what you guys think is the best solution so that we can start using it.

[hhellyer]

• I'd go for 3, but skipping the makefile and just always copying it via curl. It's 228k which isn't massive. The CI will have to copy it every time anyway as it creates a new workspace so using a makefile just to stop repeated downloads doesn't buy us that much.
• Running the tests in development will copy it but at least we'll be in sync with what the CI is likely to run. (Unless someone updates it very quickly.) All the other solutions make it easier for a copy to get out of sync with the node core version and in some cases quite a lot of work to sync up.
• It's simple.

[richardlau]

My problem with 3 is compatibility with Windows:

1. No make
2. Possibly no curl -- I think git for Windows may include curl but can we even assume git is installed?

[sam-github]

1. is the simplest, requires no network access, or proxy support, or curl (which doesn't exist on windows)

Size of the file doesn't matter, we won't even see it after the first PR. It will be in .npmignore, so it won't get published.

Its not even an onerous manual process, https://github.com/nodejs/node/commits/master/tools/cpplint.py seems to get a couple updates a year, but the curl could be added to the Makefile.

[mhdawson]

1 or 3(without the makefile and the simplest command to grab the file) would be my vote. I'd like to avoid having multiple copies to keep in sync but if making the transfer cross platform is going to be a significant effort I'm fine with 1. The other alternative would be to have the lint file in its own repo as I think we have to assume we can clone from all platforms.

[gdams]

Solution 1 is here: #47

[gibfahn]

I think the ideal solution is for node to move its js and cpp linting out into a separate repo/npm module. Then node could vendor it (the way it currently does with cpplint and eslint), and other modules who wanted to use or extend the default node lint setup could just include the module as a devDependency. So ideally option 2, but that's probably more of a long-term thing (as it would require consensus from node core).

If we're not doing that, I think it makes sense to check in the file (option 1). If the upstream (node core) cpplint.py gets updated, we probably don't want to automatically start using it, as that could start causing CI to fail for unrelated PRs (for example).