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.

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

🏗 Refactor lint.js and update jsdoc/check-tag-names rule #15298

Merged
merged 5 commits into from
May 15, 2018
Merged

🏗 Refactor lint.js and update jsdoc/check-tag-names rule #15298

merged 5 commits into from
May 15, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented May 15, 2018

This PR:

  1. Exempts well known closure tags and custom JSDoc tags from the jsdoc/check-tag-names rule
  2. Refactors the logic in lint.js so that:
    • All JS files are linted in warning mode when .eslintrc is changed
    • Only the JS files in the PR are linted in warning mode when more than 15 files are changed at a time (probably a refactor, see 🏗Add a mechanism to disable strict linting during PR checks #15291)
    • Lint results are collapsed on Travis when a large number of files are linted (the logs are copious)

Follow up to #15294
Follow up to #15256
Partial fix for #15255

@rsimha rsimha self-assigned this May 15, 2018
@rsimha rsimha requested review from jridgewell and alanorozco May 15, 2018 05:30
@rsimha rsimha changed the title 🏗 Exempt custom tags from jsdoc/check-tag-names rule 🏗 Exempt custom tags from jsdoc/check-tag-names rule; Refactor lint.js May 15, 2018
@rsimha rsimha changed the title 🏗 Exempt custom tags from jsdoc/check-tag-names rule; Refactor lint.js 🏗 Refactor lint.js and update jsdoc/check-tag-names rule May 15, 2018
@rsimha rsimha requested a review from dreamofabear May 15, 2018 14:26
@rsimha
Copy link
Contributor Author

rsimha commented May 15, 2018

/to @jridgewell @alanorozco @choumx

*
* @return {boolean}
*/
function eslintrcChangesInPr() {

Choose a reason for hiding this comment

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

Consider checking that this is a PR build inside this function instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check for a PR build needs to be outside because it's also used when .eslintrc is not in the list of files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, good idea. Done.

/**
* Enables linting in strict mode.
*/
function enableStrictMode() {

Choose a reason for hiding this comment

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

Nit: enableStrictLinting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

*
* @param {!Array<string>} files
*/
function setFiles(files) {

Choose a reason for hiding this comment

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

Nit: setFilesToLint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

*/
function setFiles(files) {
config.lintGlobs =
config.lintGlobs.filter(e => e !== '**/*.js').concat(files);

Choose a reason for hiding this comment

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

Do you want to check paths that end with **/*.js instead of strictly matching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works because there's only one positive glob in config.lintGlobs:

lintGlobs: [
'**/*.js',
'!**/*.extern.js',
'!{node_modules,build,dist,dist.3p,dist.tools,' +
'third_party}/**/*.*',
'!examples/**/*.*',
// TODO: temporary, remove when validator is up to date
'!validator/**/*.*',
'!eslint-rules/**/*.*',
'!karma.conf.js',
'!**/local-amp-chrome-extension/background.js',
'!extensions/amp-access/0.1/access-expr-impl.js',
'!extensions/amp-animation/0.1/css-expr-impl.js',
'!extensions/amp-bind/0.1/bind-expr-impl.js',
'!test/coverage/**/*.*',
],

@rsimha rsimha merged commit 32ec5af into ampproject:master May 15, 2018
@rsimha rsimha deleted the 2018-05-15-AllowedAnnotations branch May 15, 2018 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants