This repository was archived by the owner on Jun 18, 2021. It is now read-only.
Repository navigation
This repository was archived by the owner on Jun 18, 2021. It is now read-only.
How to lint c++ code #54
Description
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.
Activity
hhellyer commentedon Feb 7, 2017
richardlau commentedon Feb 8, 2017
My problem with 3 is compatibility with Windows:
makecurl-- I think git for Windows may include curl but can we even assume git is installed?sam-github commentedon Feb 8, 2017
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 commentedon Feb 9, 2017
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 commentedon Feb 9, 2017
Solution 1 is here: #47
gibfahn commentedon Feb 10, 2017
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).