Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Add tslint/prettier for linting/formatting. #299

Merged
merged 4 commits into from
Mar 31, 2018
Merged

Add tslint/prettier for linting/formatting. #299

merged 4 commits into from
Mar 31, 2018

Conversation

gurgeous
Copy link
Contributor

This change adds tslint & prettier as dev dependencies and recommends the vscode extensions as well. Using tslint will increase the quality of our work with very little dev cost. Prettier will auto-format our js and ts files on save. This sidesteps all problems related to code formatting, since prettier will just take care of it for us.

There are no code changes in this PR. Once this is merged, we should go ahead and run tslint and prettier across the entire source tree as a single PR. Otherwise we'll suffer through piecemeal diffs for subsequent PRs.

If we agree that this is a step we'd like to take, please read the proposed .prettierrc below. You can see the full list of options here:

https://prettier.io/docs/en/options.html

Personally I don't care very much about the specific prettier settings. That's the great thing about adopting prettier - using it helps you stop caring about formatting. :)

@gurgeous gurgeous mentioned this pull request Mar 14, 2018
tslint.json Outdated
@@ -0,0 +1,7 @@
{
"defaultSeverity": "error",
"extends": ["tslint:recommended", "tslint-config-prettier"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking tslint-microsoft-contrib vs tslint:recommended here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have zero experience with ts or tslint. Fine with me :)

@wingrunr21
Copy link
Collaborator

Except for using tslint:recommended this all seems fine.

@gurgeous
Copy link
Contributor Author

I looked into it and it appears that tslint-microsoft-contrib is incompatible with linting one file at a time. From https://marketplace.visualstudio.com/items?itemName=eg2.tslint, "Rules with type information are currently not supported by vscode-tslint, pls see issue https://github.com/Microsoft/vscode-tslint/issues/70."

Those rules only work if you lint the entire project at once with a grunt task...

Maybe we could start with this and switch to better rules as they become available? Better some tslint than no tslint :)

@wingrunr21
Copy link
Collaborator

I'm not following. The tslint-microsoft-contrib is just a set of pre-configured TSLint rules. Why would it be any more or less compatible with vscode-tslint than any other TSLint ruleset?

@gurgeous
Copy link
Contributor Author

My understanding of these tools is not great so I could be wrong. The tslint-microsoft-contrib rules require running with --type-check --project, otherwise you get a bunch of errors like below. But vscode-tslint doesn't support those options, because that would require linting the whole project each time linting is run.

Warning: The 'no-use-before-declare' rule requires type information.
Warning: The 'no-cookies' rule requires type information.
Warning: The 'await-promise' rule requires type information.
Warning: The 'match-default-export-name' rule requires type information.
Warning: The 'no-floating-promises' rule requires type information.
Warning: The 'no-for-in-array' rule requires type information.
Warning: The 'no-unsafe-any' rule requires type information.
Warning: The 'promise-function-async' rule requires type information.
Warning: The 'restrict-plus-operands' rule requires type information.
Warning: The 'strict-boolean-expressions' rule requires type information.
Warning: The 'completed-docs' rule requires type information.
Warning: The 'no-unnecessary-qualifier' rule requires type information.
Warning: The 'no-unnecessary-type-assertion' rule requires type information.
Warning: The 'no-void-expression' rule requires type information.
Warning: The 'use-default-type-parameter' rule requires type information.

@wingrunr21
Copy link
Collaborator

Yep, got that. That just means we are going to need to support also having people being able to run TSLint over the whole project (possibly via a task that can be run within VSCode so you still get feedback in the problems section). vscode-tslint's explanation for not supporting this is valid, but if we are running TSLint we should be linting types too IMO

@gurgeous
Copy link
Contributor Author

I think there's value in tslint regardless of the ruleset applied. If you think the tslint-microsoft-contrib rules are worth the bother I'm happy to give it a shot. Certainly we can turn it on and then just ignore the warnings due to lack of types.

We can add a task to run with --type-check etc. too. Separate PR, probably.

Let me try turning it on here and see how it feels. Maybe we can disable the invalid rules in the vscode-tslint config to avoid warning spam.

@wingrunr21
Copy link
Collaborator

Please switch it to tslint-microsoft-contrib

@gurgeous
Copy link
Contributor Author

Sure. How about this? I disabled a few rules that were overly noisy. I think it's generating some useful stuff now. If you have some experience with tslint it would be great if you could help me customize the ruleset a bit...

@gurgeous
Copy link
Contributor Author

Oh, and here's a sample of the errors produced:

$ tslint --project src --config tslint.json

255 Unsafe use of expression of type 'xxx'.
 142 expected variable-declaration: 'xxx' to have a typedef
 107 expected arrow-parameter: 'xxx' to have a typedef
  91 Identifier 'xxx' is never reassigned; use 'xxx' instead of 'xxx'.
  35 Forbidden 'xxx' keyword, use 'xxx' or 'xxx' instead
  30 expected parameter: 'xxx' to have a typedef
  30 Missing blank line before return
  26 Use a template literal instead of concatenating with a string literal.
  25 if statements must be braced
  25 expected call-signature: 'xxx' to have a typedef
  23 expected arrow-call-signature to have a typedef
  20 Type declaration of 'xxx' loses type-safety. Consider replacing it with a more precise type.
  18 variable name must be in lowerCamelCase or UPPER_CASE
  18 Operands of 'xxx' operation must either be both strings or both numbers, consider using template literals
  18 Import sources within a group must be alphabetized.
  17 functions that return promises must be async
  14 Expression has type `void`. Put it on its own line as a statement.
  13 Replace block comment with a single-line comment
  13 Promises must be handled appropriately
... (a hundred more) ...

@gurgeous
Copy link
Contributor Author

BTW, this is the default config that yo code generates for new ts vscode extensions:

{
  "rules": {
    "no-string-throw": true,
    "no-unused-expression": true,
    "no-duplicate-variable": true,
    "curly": true,
    "class-name": true,
    "semicolon": [true, "always", "ignore-bound-class-methods"],
    "triple-equals": true
  },
  "defaultSeverity": "warning"
}

Might be worth considering. Let me know if you want me to fix these conflicts or make changes to get it merged.

@wingrunr21 wingrunr21 merged commit 0df353f into rubyide:master Mar 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants