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.

Sign up for GitHub

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

Jump to bottom

Introduce npm run lint command for cpp formatting #752

Closed
wants to merge 1 commit into from
Closed

Introduce npm run lint command for cpp formatting #752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/lint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const util = require('../lib/util')

const lint = (options = {}) => {
util.lint()
}

module.exports = lint
9 changes: 9 additions & 0 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,15 @@ const util = {
util.run('ninja', ['-C', config.outputDir, config.buildTarget], options)
},

lint: (options = {}) => {
console.log('linting ' + config.projects['brave-core'].dir + '...')
options.cwd = config.projects['brave-core'].dir
options = mergeWithDefault(options)
// git cl format checks rietveld.server is set. Just set null.
Copy link
Contributor

Choose a reason for hiding this comment

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

what is rietveld.server?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is code review site of chromium project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about what git cl has to do with lint. We want to call vendor/depot_tools/cpplint.py in some kind of push hook

Copy link
Member Author

@simonhong simonhong Aug 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't read full git_cl.py, but it seems it uses cpplint.py for lint internally. it seems it utilizes clang_format.py.
How about providing two kinds of formatting ways - npm run lint and git hook?

Copy link
Member Author

Choose a reason for hiding this comment

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

git cl is utilities for chromium code review - https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-cl.html
It has many subcommands and we can easily use format or lint subcommand for our formatting

Copy link
Contributor

Choose a reason for hiding this comment

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

but there's no changelist to run it on. We need it to run on directory. I just tried to run and got Cannot lint an empty CL
we also need to make sure we're using cpplint.py in all cases

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you have any modification in src/brave?
That command tries to reformat files that has changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

lint should fail right now whether there are changes or not because I've seen dozens of what would be lint errors in the code

Copy link
Member Author

Choose a reason for hiding this comment

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

If you use format it will detect the errors.
As I told you in DM, if we want to use cpplint.py instead of clang_format, git cl would not be the good choice.

util.run('git', ['config', 'rietveld.server', 'null'], options)
util.run('git', ['cl', 'format'], options)
},

submoduleSync: (options = {}) => {
if (!options.cwd) options.cwd = config.rootDir // default cwd `./src` may not exist yet
options = mergeWithDefault(options)
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"push_l10n": "node ./scripts/commands.js push_l10n",
"pull_l10n": "node ./scripts/commands.js pull_l10n",
"chromium_rebase_l10n": "node ./scripts/commands.js chromium_rebase_l10n",
"test": "node ./scripts/commands.js test"
"test": "node ./scripts/commands.js test",
"lint": "node ./scripts/commands.js lint"
},
"config": {
"projects": {
Expand Down
5 changes: 5 additions & 0 deletions scripts/commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const chromiumRebaseL10n = require('../lib/chromiumRebaseL10n')
const createDist = require('../lib/createDist')
const upload = require('../lib/upload')
const test = require('../lib/test')
const lint = require('../lib/lint')
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe you moved it to util but this is still around and it should get it from util instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

did you forget to add lint.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. I forgot to add lint.js. Added.


program
.version(process.env.npm_package_version)
Expand Down Expand Up @@ -105,5 +106,9 @@ program
.arguments('[build_config]')
.action(test)

program
.command('lint')
.action(lint)

program
.parse(process.argv)