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

[tslint] lint typescript code #19105

Merged
merged 6 commits into from
May 22, 2018
Merged

Conversation

spalger
Copy link
Contributor

@spalger spalger commented May 16, 2018

Fixes #18780

Adds TSLint integration.

Running

node scripts/tslint will run the linter on all TypeScript "projects" (defined by tsconfig.json files). This script is also run as a part of testing anywhere eslint is run.

2018-05-17 15 09 08

Rules

In order to avoid lengthy debate about which rules to enable we have enabled:

  • tslint:recommended preset
  • no-unused-variable rule (requires type information so it's not a part of the preset)
  • prettier

We can open issues and have discussions about evolving these selections as we go.

Along with the default rules, a custom rule to ensure that license headers are added to files in the x-pack directory is in use.

To avoid a massive number of changes in this PR tslint.yaml files have been added to the existing TypeScript projects to disable any rules that have violations.

Type Checking

TSLint deprecated the ability to type-check while linting, and it's currently providing a different output than we get with the build because it includes files that are not shipped with the build. Rather than block TypeScript integration on this I discussed the options with @azasypkin and we both agree that the type checking done at build time is sufficient as a final check that must pass before files can be committed, that editor integrations are okay for development, and that people can run the tsc directly if they want more comprehensive type checking during development:

yarn tsc --noEmit --watch --pretty --project ./relative/path/to/tsconfig.json

@spalger spalger changed the base branch from implement/typescript to master May 16, 2018 02:10
@spalger spalger force-pushed the implement/tslint branch 5 times, most recently from 96ccc5b to 8d8b66b Compare May 17, 2018 22:16
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elastic elastic deleted a comment from elasticmachine May 21, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

for (const Linter of [Eslint, Tslint]) {
const filesToLint = Linter.pickFilesToLint(log, files);
if (filesToLint.length > 0) {
await Linter.lintFiles(log, filesToLint);
Copy link
Contributor

Choose a reason for hiding this comment

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

This current implementation doesn't attempt to run tslint at all if there is an error from eslint, so if you have lint failures in both .js and .ts files, you won't know that until you attempt to commit again after fixing the .js files. It should probably run both commands and show the results from each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the same is true for file casing issues and eslint. If there are casing issues eslint is never run.

I'll put up a separate PR that updates the precommit hook to clearly log errors from multiple tasks and collect those errors into an appropriate exit code, but I personally think it's out of the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weltenwort
Copy link
Member

I failed to get this to pick up projects located in ../kibana-extra/* even though they are listed in the kibana.dev.yml. Is this outside of the scope of this tool or do I need to change the invocation in some way?

@spalger
Copy link
Contributor Author

spalger commented May 22, 2018

@weltenwort just like our current eslint setup this isn't intended to lint things in ../kibana-extra/*. It's not an unrealistic feature request though...

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor Author

spalger commented May 22, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger spalger merged commit 4c2a90d into elastic:master May 22, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request May 22, 2018
* [tslint] lint typescript code

* [tslint] filter projects when running specific project

* [dev/ts] use more explicit types

* [dev/ts] add note about why using glob

* [dev/ts] rely on ts, use fewer getters
spalger pushed a commit that referenced this pull request May 22, 2018
Backports the following commits to 6.x:
 - [tslint] lint typescript code  (#19105)
@spalger
Copy link
Contributor Author

spalger commented May 22, 2018

6.x/6.4: 37487a8

@spalger spalger deleted the implement/tslint branch May 22, 2018 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Operations Team label for Operations Team v6.4.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants