🏗 During PR checks, lint only the files in the PR, and flag errors instead of warnings #15256
Conversation
rsimha
changed the title
.eslintrc
Outdated
| @@ -123,6 +123,12 @@ | |||
| "space-in-parens": 2, | |||
| "space-infix-ops": 2, | |||
| "space-unary-ops": [1, { "words": true, "nonwords": false }], | |||
| "valid-jsdoc": [1, { | |||
| "requireParamDescription": false, | |||
There was a problem hiding this comment.
Add:
"prefer": { "return": "return" }
.eslintrc
Outdated
| "valid-jsdoc": [1, { | ||
| "requireParamDescription": false, | ||
| "requireReturn": false, | ||
| "requireReturnType": false, |
There was a problem hiding this comment.
This should be true, right? We always want return types, if there's a return tag.
build-system/tasks/lint.js
Outdated
| // Override .eslintrc settings here. | ||
| options['rules'] = { | ||
| // TODO(rsimha, #15255): This should error by default in .eslintrc. | ||
| 'valid-jsdoc': 2, |
There was a problem hiding this comment.
This removes the options we passed to the eslint configuration.
|
@jridgewell Comments addressed. PTAL. |
| const filesInPr = | ||
| getStdout('git diff --name-only master...HEAD').trim().split('\n'); | ||
| return filesInPr.filter(function(file) { | ||
| return path.extname(file) == '.js'; |
There was a problem hiding this comment.
In a future change, do you mind adding '.ts' to this list or making a utility method the build process uses for filtering files based on extension?
There was a problem hiding this comment.
Good suggestion. Assuming it makes sense to run eslint checks on .ts files, we should definitely include them in gulp lint. Will test this out via a separate PR (there are other places that need to change too, so it's kinda out of scope for this PR.)
|
Might be naïve of me, but if an export changes shape during a PR would other files need to be verified that leverage the exported object? |
|
@kristoferbaxter You're right that a change to an export will affect other files. Those checks are already incorporated in |
|
Thanks @rsimha. Looks good to me. |
build-system/tasks/lint.js
Outdated
| // Override .eslintrc settings here. | ||
| options['rules'] = { | ||
| // TODO(rsimha, #15255): Make this error by default in .eslintrc. | ||
| 'valid-jsdoc': [2, { |
There was a problem hiding this comment.
We could instead maintain this as a second .eslintrc (let's say, .eslintrc-strict) with an extends option pointing to the default `.eslintrc. Then, switch to using the strict version for PRs.
This moves this configuration out of JavaScript code.
|
I bet a lot of these are line breaks within a type definition. Closure Compiler doesn't seem to mind but VSCode and GitHub barf on them. |
|
@choumx You may be right about line breaks within a type definition. However, I saw errors due to line breaks before type definitions that were being completely hidden from Fixes coming up. |
|
@jridgewell Changes made. PTAL. |
There are upwards of 1300 JSDoc errors in the AMP source code. There's no easy way to fix all of them in one go.
This PR changes the way
gulp lintworks during PR checks and when it is run locally viagulp lint --filesor as part ofgulp pr-check:gulp pr-check, instead of linting the entire code base, we now lint only the JS files touched by the PR, with file-level exemptions in place..eslint-strict.With this, if someone were to touch a file, they will be required to fix the existing JSDoc errors before
gulp lintwill pass for the PR check. This way, widespread errors in our code will be gradually fixed as more files are touched.Partial fix for #15255
Required in order to fix #14392
Required in order to fix #10277
Follow up to #15247