build: configure ESLint coverage for gulp, scripts, docs, config #11064

Splaktar · 2018-01-12T10:48:58Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[x] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[x] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently none of the repository is linted with ESLint except for src/. This means that all of the scripts and code in config/ docs/ gulp/ scripts/ is not covered. This code contains quite a wide range of bad practices. Additionally, there is an informal mix of ES5 and ES6 in the repository.

configure parts of the repo for ES5 (src, docs/app/js)
configure parts of the repo for ES6 (scripts, gulp, config, root)
remove unused committers.json
clean up ESLint errors
some minor ES6 updates and refactoring outside of src/

Issue Number:
N/A

What is the new behavior?

Add linting coverage for the vast majority of the repository.
Explicitly configure different sections of the repo for ES5 or ES6 and run linting to verify that no crossover violations occur (breaking browser support).

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

N/A

Splaktar · 2018-01-12T10:57:16Z

@graingert As you implemented the initial ESLint support and have another ESLint PR in flight, can you please take a look at this PR and suggest any possible improvements or best practices for configuring ESLint.

I put this together while reviewing all of the tooling and scripting in the project so that I could better understand it prior to putting out the 1.1.6 release.

Run all of the gulp tasks successfully
Manually test and verify that the docs site functioned and looks as expected
Run the release script in dryRun mode and verify that it completes properly
Run npm run lint and verify that there are no failures

graingert · 2018-01-12T10:59:59Z

docs/app/js/.eslintrc.json

@@ -0,0 +1,274 @@
+{
+  "extends": "eslint:recommended",


eslint configs are automatically inherited up the directory tree. So you can remove duplicate rules.

Aaaalso you can use the "overrides" param in the root .eslintrc. (prefered)

graingert · 2018-01-12T11:01:05Z

docs/app/js/.eslintrc.json

+    "CryptoJS": true,
+    "hljs": true
+  },
+  "overrides": [


eg this overrides list can add an object with files: ["docs/"] configured for 'docs'.

I see that overrides is an array... oh and it has a seperate rules, globals, and env that applies to that set of files. Oh very nice. Then I can do it all in that one file, excellent. I'll work on merging things together. Thank you!

graingert · 2018-01-12T11:02:07Z

docs/app/js/highlight-angular.js

@@ -53,6 +53,7 @@ angular.module('docsApp')

          var highlightedCode = hljs.highlight(attr.language || attr.lang, lines.join('\n'), true);
          highlightedCode.value = highlightedCode.value
+            // eslint-disable-next-line no-div-regex
            .replace(/=<span class="hljs-value">""<\/span>/gi, '')


should be possible to do \= here

That gives me no-useless-escape error. I think that I am just going to disable no-div-regex globally.

graingert · 2018-01-12T11:07:03Z

docs/config/index.js

@@ -42,6 +42,7 @@ module.exports = new Package('angular-md', [

  computeIdsProcessor.idTemplates.push({
    docTypes: ['content'],
+    // eslint-disable-next-line no-template-curly-in-string
    idTemplate: 'content-${fileInfo.relativePath.replace("/","-")}',


idTemplate: { getId: ({ fileInfo }) => `content-${fileInfo.relativePath.replace("/","-")}` }

graingert · 2018-01-12T11:08:08Z

src/.eslintrc.json

@@ -0,0 +1,273 @@
+{
+  "extends": "eslint:recommended",


same here, use 'overrides'

graingert · 2018-01-12T11:08:50Z

scripts/write-presubmit-scheduler.js

@@ -19,6 +19,7 @@ const logDir = '/tmp/v1-pr-presubmit-logs';
 * The default path is stored in an environment variable because it references an internal-Google
 * location.
 */
+// eslint-disable-next-line no-process-env


ignore this in the 'scripts' overrides.

graingert · 2018-01-12T11:12:07Z

updateVersionPicker.js

@@ -47,23 +44,20 @@



just use

`cp -r ${docs.latest} latest`,

graingert · 2018-01-12T11:16:36Z

updateVersionPicker.js

-    return str.replace(/\{\{[^\}]+\}\}/g, function (match) {
+  function fill (str) {
+    return str.replace(/{{[^}]+}}/g, function (match) {
+      // eslint-disable-next-line no-eval
      return eval(match.substr(2, match.length - 4));


instead of eval just use template string functions.

actually this is only used for cp -r ${docs.latest} latest, so just totally not-needed.

graingert · 2018-01-12T11:18:20Z

.eslintignore

@@ -0,0 +1,3 @@
+src/core/services/compiler/compiler.spec.js


you could probably do these with the 'overrides' feature.

I tried to add them to an excludedFiles array, but that didn't work. Most of them cause ESLint to actually throw a parsing error if the file is evaluated at all. Adding them to the ignore file avoided this issue.

test/ doesn't seem to have that much wrong with it. I suspect they can be added to the overrides.2.files array.

I started into cleaning those up and getting them passing, but there were a number of things that I am not comfortable changing at this time. I'd like to keep that as part of a separate PR since those files are actually used to test the library and changing them would make this PR no longer 'merge safe'.

graingert · 2018-01-12T11:19:22Z

@Splaktar good idea to upgrade all the util scripts to modern js. Looks like there was a bit of a misunderstanding on how .eslintrc inheritance/overrides operates.

Splaktar · 2018-01-13T07:44:11Z

Yep, thank you very much for the review. I had a feeling that it may have some inheritance features like TSLint, I just didn't have time to research every part of how it worked.

graingert · 2018-01-15T10:02:45Z

.eslintrc.json

@@ -1,271 +1,312 @@
 {
-    "extends": "eslint:recommended",


I think it would be convenient to not change the indenting in this PR so there's fewer rebase issues with my document/window PR, and so it's easier to review changes here.

You have a good point there. Can you please update your PR to use 2 spaces instead of 4? That should minimize most of it. I'm not sure if/when, we'll be able to get that in as it doesn't have non-zero risk. I would rather not keep this file using inconsistent 4 space formatting for an extended period of time.

I'm not sure if/when, we'll be able to get that in as it doesn't have non-zero risk

as it has non-zero risk?

I'd be happy with the indent change happening in this PR as long as it's in one atomic commit at the end

It's currently impossible to review

OK, I understand. That's the hard thing with the original formatting not being the same as the project. I guess it's using CLANG for formatting atm (which I don't actually use). I'm not sure why .editorconfig isn't used other than Google is used to CLANG. Either way, I don't have your original formatting settings. I'll set it back to 4 spaces and see how that looks.

Pushed. Yeah, the diff looks much better now. Please take a look.

devversion · 2018-01-16T15:05:05Z

gulp/config.js

@@ -1,5 +1,6 @@
 var args = require('minimist')(process.argv.slice(2));
-var VERSION = args.version || require('../package.json').version;
+var version = require('../package.json').version;
+var VERSION = args.version || version;


This is confusing and ambiguous. I'd prefer just inlining this.

var version = args.version || require('../package.json').version;

Also if you update it like that in the const.js file, you can just import the VERSION export here. This removes duplicate logic.

that breaks the top level require only rule.

and you can't use any of the wacky conditional require stuff with esm, so it's good to require top level imports only.

maybe use:

const pkg = require('../package.json'); const VERSION = args.version || pkg.version;

Yeah, the important thing is just that the version and VERSION names are way too similar and should be somehow differentiated.

devversion · 2018-01-16T15:07:26Z

gulp/tasks/karma-fast.js

@@ -30,14 +29,15 @@ exports.task = function (done) {

  gutil.log('Running unit tests on unminified source.');

-  karma = new Server(karmaConfig, captureError(clearEnv,clearEnv));
+  var karma = new Server(karmaConfig, captureError(clearEnv,clearEnv));


Is it intended to use var here? It kind of feels inconsistent to me (why not const)

it's odd that it didn't complain.

I updated scripts/ but I didn't update all of the gulp tasks yet...

probably worth running the safe transforms

Yep, done. Pushing soon.

graingert · 2018-01-17T10:52:37Z

package.json

@@ -85,6 +85,6 @@
    "docs:watch": "gulp watch site --dev",
    "test:fast": "gulp karma-fast",
    "test:full": "gulp karma",
-    "lint": "eslint src"
+    "lint": "eslint src docs gulp config scripts"


could probably be eslint .

Would that try to lint dist/?

I believe eslint also uses .gitignore by default.

ah no it doesn't you need:

eslint . --ignore-path .gitignore --ignore-path .eslintignore

ah, it looks like you can't set two ignore files. Probably worth just adding dist to .eslintignore.

eslint/eslint#9794

Updated and pushed.

configure parts of the repo for ES5 (src, docs/app/js) configure parts of the repo for ES6 (scripts, gulp, config) remove unused committers.json clean up ESLint errors

graingert · 2018-01-17T12:26:43Z

Makes sense.

…

On 17 Jan 2018 11:29, "Michael Prentice" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .eslintignore <#11064 (comment)>: > @@ -0,0 +1,3 @@ +src/core/services/compiler/compiler.spec.js I started into cleaning those up and getting them passing, but there were a number of things that I am not comfortable changing at this time. I'd like to keep that as part of a separate PR since those files are actually used to test the library and changing them would make this PR no longer 'merge safe'. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11064 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZQTE5Nmw20METLMf_bVq7uyqHSutZ-ks5tLdmjgaJpZM4RcHuF> .

Splaktar · 2018-01-18T10:21:13Z

I ran into some issues testing the build-contributors task. After debugging, it uncovered mgechev/github-contributors-list#12. I opened mgechev/github-contributors-list#13 to fix the issue.

This should be good to go now.

…ular#11064)

Splaktar added this to the 1.1.6 milestone Jan 12, 2018

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 12, 2018

Splaktar added type: build needs: review This PR is waiting on review from the team type: refactor labels Jan 12, 2018

Splaktar requested a review from devversion January 12, 2018 10:50

graingert suggested changes Jan 12, 2018

View reviewed changes

graingert reviewed Jan 12, 2018

View reviewed changes

Splaktar force-pushed the cleanupScripts branch from 74be5f9 to 32eb3d5 Compare January 13, 2018 09:07

Splaktar removed the needs: review This PR is waiting on review from the team label Jan 13, 2018

Splaktar force-pushed the cleanupScripts branch 3 times, most recently from 879822d to d6668a7 Compare January 13, 2018 10:09

Splaktar assigned andrewseguin Jan 13, 2018

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: merge safe labels Jan 13, 2018

graingert reviewed Jan 15, 2018

View reviewed changes

devversion reviewed Jan 16, 2018

View reviewed changes

Splaktar modified the milestones: 1.1.6, 1.1.7 Jan 17, 2018

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed pr: merge ready This PR is ready for a caretaker to review labels Jan 17, 2018

Splaktar force-pushed the cleanupScripts branch from d6668a7 to 614e18e Compare January 17, 2018 10:24

graingert reviewed Jan 17, 2018

View reviewed changes

build: configure ESLint coverage for gulp, scripts, docs, config

383738a

configure parts of the repo for ES5 (src, docs/app/js) configure parts of the repo for ES6 (scripts, gulp, config) remove unused committers.json clean up ESLint errors

Splaktar force-pushed the cleanupScripts branch from 614e18e to 383738a Compare January 17, 2018 11:23

graingert approved these changes Jan 17, 2018

View reviewed changes

Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jan 18, 2018

Splaktar assigned josephperrott and unassigned andrewseguin Jan 18, 2018

josephperrott merged commit 6e425c4 into angular:master Jan 18, 2018

Splaktar deleted the cleanupScripts branch January 27, 2018 07:17

chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018

build: configure ESLint coverage for gulp, scripts, docs, config (ang…

c541d9d

…ular#11064)

build: configure ESLint coverage for gulp, scripts, docs, config #11064

build: configure ESLint coverage for gulp, scripts, docs, config #11064

Conversation

Splaktar commented Jan 12, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Splaktar commented Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Jan 12, 2018

Splaktar commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

devversion Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Jan 17, 2018 via email

Splaktar commented Jan 18, 2018

Splaktar commented Jan 12, 2018 •

edited

Loading

graingert Jan 12, 2018 •

edited

Loading

graingert Jan 12, 2018 •

edited

Loading

graingert Jan 12, 2018 •

edited

Loading

graingert Jan 16, 2018 •

edited

Loading

devversion Jan 16, 2018 •

edited

Loading