Switching to ESLint #181
Switching to ESLint #181
Conversation
cbraynor
force-pushed
the
cbraynor/eslint
branch
from
b18baaf to
3b153b6
Compare
.eslintignore
Outdated
| lib/** | ||
| src/testdata/** | ||
| tests/** | ||
| **/webpack.config.js |
There was a problem hiding this comment.
Please add runner/dist/** to this list. If that directory is present (because you've built the runner locally) then it basically just hangs. I gave up and killed it after 10 minutes.
I think it's a combination of the prettier/prettier rule and the runner/dist/codeql-runner.js having so many error that it takes so long to lint it's worthless. If I ignore this directory then everything works as expected and linting takes about 7 seconds.
For fun, if I instead disable prettier/prettier then the other rules are able to complete in about 14 seconds, and report 3837 errors...
There was a problem hiding this comment.
Ah, I'd not run into that myself, good to know. I'll add this to the ignore file
|
I see that the bulk of the changes to actual code are formatting. It's changing single quotes to double quotes throughout, and moving braces and parameteres around. I don't really have a problem with this, as in I'm fine with any style guide so long as it's consistent and enforced. It is quite a big change though, so should we consult with the rest of the team? |
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Contents of .eslintrc.json looks fine to me. As said elsewhere, adding a script so we can do npm run lint-fix would be great. Other than that and the addition to .eslintignore, this is good to go whenever you're happy.
I've skimmed through some of the src files and the changes all look safe to me, so I think we should go with what's there for now. I have found one or two formatting choices that are a bit questionable, for example
logger.debug(
`Unable to compute fingerprint for invalid location: ${JSON.stringify(
primaryLocation
)}`
);but I'm not sure it's useful to worry about that now. An autoformatter is never going to do exactly what you might like in every situation, but so long as it is enforcing the style we can maybe tweak it in the future if we want to.
cbraynor
force-pushed
the
cbraynor/eslint
branch
from
3b153b6 to
7bdf90a
Compare
|
Recreated the same steps on the current master to avoid having to deal with the merge - should be the same except for addressing the review comments |
This switches us away from the deprecated TSLint with an aspirational
.eslintrc.jsonfile. I've put a bunch of overrides in that we can slowly chip away at in separate PRs.Recommend you review commit by commit and mostly ignore the
node_modulesupdate / mergeMerge / deployment checklist