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

chore: separate tslint config files for build and vscode #969

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Feb 6, 2018

This is a follow-up to #964.

Two config files are in place now:

  • tslint.build.json for CLI build scripts
  • tslint.json for vscode

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@raymondfeng raymondfeng force-pushed the fix-tslint-vscode branch 2 times, most recently from 37487a2 to 951616f Compare February 6, 2018 17:29
@raymondfeng raymondfeng requested a review from virkt25 February 6, 2018 17:29
Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

IIUC, this is an undo of the previous commit. But we will end up without a check for unused import again.

Is it possible to update the package.json "tslint" script to pass it tslint.build.json as the config? (And that might remove the warning for tslint plugin for VSCode).

@raymondfeng
Copy link
Contributor Author

IIUC, this is an undo of the previous commit. But we will end up without a check for unused import again.

Not really. Please note that CLI build uses tslint.build.json and it catches violation of unused vars.

@virkt25
Copy link
Contributor

virkt25 commented Feb 7, 2018

By CLI do you mean @loopback/build and the package.json file?

@raymondfeng
Copy link
Contributor Author

By CLI do you mean @loopback/build and the package.json file?

npm run lint which uses lb-tslint.

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

I see. So it doesn't pick it up the the top level tslint.json which extends tslint.common.json? That seems strange but ok. As long as it catches unused imports :)

Maybe wait till @bajtos / @kjdelisle have a chance to review this as well before merging.

@raymondfeng
Copy link
Contributor Author

Yes, lb-tslint uses tslint.build.json over tslint.json. This way, vscode uses tslint.json while our build uses tslint.build.json.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

This is great, thank you @raymondfeng for fixing the problems introduced by your previous PR 👏

I have checked out your feature branch and verified that:

  • vscode-tslint does not report any warnings
  • unused variables are reported by npm run tslint

I think you have a bug in the tslint.build.json template (see my comment below).

Other than that, this patch LGTM 👍

"no-floating-promises": true,
"no-void-expression": [true, "ignore-arrow-function-shorthand"]
}
"extends": ["./node_modules/@loopback/build/config/tsconfig.build.json"]
Copy link
Member

Choose a reason for hiding this comment

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

I think you have a typo here - are you sure that tslint.build.json should be extending tsconfig.build.json? I think you meant to extend tslint.build.json 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.

Good catch. Fixed.

This is a follow-up to #964.

Two config files are in place now:
- tslint.build.json for CLI build scripts
- tslint.json for vscode
@raymondfeng raymondfeng merged commit c86b0cf into master Feb 8, 2018
@bajtos bajtos deleted the fix-tslint-vscode branch February 9, 2018 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants