-
Notifications
You must be signed in to change notification settings - Fork 45
Add lint rules from main nodejs project. #44
Comments
There was some previous discussion of this in #15. https://github.com/nodejs/node uses eslint (with custom rules) for JavaScript and cpplint.py for C++ (majority of node(-)report is C++). We most definitely want to follow the same rules to reduce the barriers to entry for contributors and to smooth future integration of this project with the runtime. For JavaScript, @geek offered a PR (#15 (comment)) but this was raised against my fork of nodereport instead of this repository. n.b. https://github.com/nodejs/node-inspect did something similar (nodejs/node-inspect@d669dc5) |
We currently have #45 and #46 as two different approaches to linting the JavaScript source, but neither of them add the custom eslint rules in https://github.com/nodejs/node/tree/master/tools/eslint-rules. Do we need those? |
I think the rules we apply should be exactly the same as applied to node core. |
@mhdawson I updated node-style to be exactly the same as the rules applied to node core. It was missing the custom node-core rules before, those are now included. |
Please do not copy core's styles. They exist in that specific form only for legacy reasons. |
I also think that we should stick to the rules used by core. |
Went ahead and approved #45 (review) |
@Fishrock123 could you be more specific about which rules you think shouldn't be followed? |
Nodereport should have a lint target developers can run to check the source code formatting rather that having reviewers find the issues.
This would help prevent pull requests from being cluttered up with formatting changes and allow us to focus more on the technical discussions.
Since nodereport (node-report) is a project under Nodejs the rules should be copied over from there so the format is consistent with the main project.
The text was updated successfully, but these errors were encountered: