New feature: lint js files #1515

YuichiNukiyama · 2016-08-25T13:42:29Z

Related issue

Fixes #515

Usage

I add jsRule option to tslint.json. We can distinguish the rules that apply to js or ts depending on this option.

{
    "jsRule": {
        "class-name": true
    }
}

Good point

simple implimentation
this is not breaking change
It easier to distinguish the rules that apply to js or ts

Bad point

There is no way to tell each rules which file(ts or js) call them
There is a rule that can not be applied to js (ex. arrow-parens rule can't apply js files)
The greater the overlap in the configuration file in some cases

Note

Now, I test only class-name and curly rule. If you think this PR is good, I add tests for another rules.

palantirtech · 2016-08-25T13:42:30Z

Thanks for your interest in palantir/tslint, @YuichiNukiyama! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

jkillian · 2016-08-29T21:22:34Z

@YuichiNukiyama What do you think of a system where each rule has metadata that's something like typescriptOnly: false? We could still keep jsRules, but that would only be as an override. By default TSLint would use all the settings in rules when linting JS files if no configuration is specified in jsRules.

YuichiNukiyama · 2016-08-30T14:43:35Z

@jkillian Metadata is good idea. But I won't use all the settings in rules when there is no jsRules option. A lot of developer use other linter for linting js. So we should use unobtrusive setting. If they don't specified any settings in jsRule, We lint nothing.

jkillian · 2016-09-01T00:43:35Z

@YuichiNukiyama But why would they pass their .js files to TSLint if they were linting them with another tool? It seems to me that if they provide .js files, they should get linted with the regular rules unless they opt-out by overriding a setting in jsRules

YuichiNukiyama · 2016-09-01T03:16:43Z

@jkillian You've given a misunderstanding ,because of my poor english. Since the TypeScript-only rule is large, the error occurs frequently when we apply the all rule options. And now many developer use eslint to lint js files. I afraid that these developer apply the jslint to js.

jkillian · 2016-09-02T16:23:20Z

Let's look at a concrete example to see what makes the most sense.

Say we start with this configuration. We then automatically skip any rules that only make sense for TS. For example, typedef-whitespace and no-namespace. We then, by default, apply the remaining rules to JS files being linted.

Are you saying that this wouldn't work well because a lot of the remaining rules wouldn't work correctly on JS files?

Sorry for my large commit.

YuichiNukiyama · 2016-09-04T11:22:03Z

@jkillian I add tests for all rules.
We can use following rules (sorry for my large commit).

alignRule
banRule
classNameRule
curlyRule
eoflineRule
fileHeaderRule
forinRule
indentRule
jsdocFormatRule
labelPositionRule
labelUndefinedRule
linebreakStyleRule
maxFileLineCountRule
maxLineLengthRule
newParensRule
noArgRule
noBitwiseRule
noConditionalAssignmentRule
noConsecutiveBlankLinesRule
noConsoleRule
noConstructRule
noDebuggerRule
noDefaultExportRule
noDuplicateKeyRule
noDuplicateVariableRule
noEmptyRule
noEvalRule
noForInArrayRule
noInvalidThisRule
noNullKeywordRule
noReferenceRule
noShadowedVariableRule
noStringLiteralRule
noSwitchCaseFallThroughRule
noTrailingWhitespaceRule
noUnreachableRule
noUnsafeFinallyRule
noUnusedExpressionRule
noUnusedNewRule
noUseBeforeDeclareRule
noVarKeywordRule
objectLiteralKeyQuotesRule
objectLiteralShorthandRule
objectLiteralSortKeysRule
oneLineRule
oneVariablePerDeclarationRule
onlyArrowFunctionsRule
orderedImportsRule
quotemarkRule
radixRule
restrictPlusOperandsRule
semicolonRule
switchDefaultRule
trailingCommaRule
tripleEqualsRule
useIsnanRule
useStrictRule
variableNameRule
whitespaceRule

YuichiNukiyama · 2016-09-10T02:40:26Z

@jkillian I add JS recommendations.

jkillian · 2016-09-16T02:08:09Z

Sorry @YuichiNukiyama, things have been really busy lately! @adidahiya, do you have time to weigh in on this / take a look?

adidahiya · 2016-10-27T17:19:43Z

@YuichiNukiyama we should have time to review this soon, I think @nchen63 can take a look. Could you resolve the merge conflicts?

YuichiNukiyama · 2016-10-29T00:59:05Z

@adidahiya Sure. I resolved conflict.
@nchen63 Please review my PR.

nchen63 · 2016-10-29T02:27:05Z

src/rules/commentFormatRule.ts

@@ -47,6 +47,7 @@ export class Rule extends Lint.Rules.AbstractRule {
        },
        optionExamples: ['[true, "check-space", "check-lowercase"]'],
        type: "style",
+        typescriptOnly: true,


why is this typescript only?

This Rule don't work well in js.

I'm ok with the PR going in with this rule disabled for js since it's not that hard to add later, but I'm just curious why you think it doesn't work well with js

Sorry, this rule pass a test. Now, #1635 privent me from doing test...

nchen63 · 2016-10-29T02:29:43Z

src/rules/arrowParensRule.ts

@@ -29,6 +29,7 @@ export class Rule extends Lint.Rules.AbstractRule {
        options: null,
        optionExamples: ["true"],
        type: "style",
+        typescriptOnly: true,


can this rule work with js also?

This Rule don't work well in js.

arrow functions supported in es6

nchen63 · 2016-10-29T02:36:06Z

src/rules/noRequireImportsRule.ts

@@ -29,6 +29,7 @@ export class Rule extends Lint.Rules.AbstractRule {
        options: null,
        optionExamples: ["true"],
        type: "maintainability",
+        typescriptOnly: true,


This Rule don't work well in js.

also useful in es6 with the new imports

YuichiNukiyama · 2016-10-29T03:22:11Z

@nchen63
Some of the rules don' work well in js file. If we want use these rules in js, We should fill rules itself.
But I think that isn't scope of this PR.

nchen63 · 2016-10-31T15:21:29Z

src/configs/recommended.ts

+        "trace",
+    ],
+    "no-construct": true,
+    "no-constructor-vars": false,


this was renamed to no-parameter-properties

parameter properties is typescript only feature. So, I removed it.

nchen63 · 2016-10-31T15:21:55Z

src/configs/recommended.ts

+    "label-position": true,
+    "label-undefined": true,
+    "max-line-length": [true, 120],
+    "member-access": true,


does this ever rule make sense for JS?

no, it's not supported in ES.

I removed this.

nchen63 · 2016-10-31T15:51:15Z

src/configs/recommended.ts

+    "no-unused-new": true,
+    // disable this rule as it is very heavy performance-wise and not that useful
+    "no-use-before-declare": false,
+    "no-var-keyword": true,


this only applies to ES6. I don't think most Javascript users will be targeting that version

I removed this.

nchen63 · 2016-11-03T00:50:22Z

Loading tslint.json that extends another config such as "recommended", won't load the base js rules. Please update extendConfigurationFile() to include them

nchen63 · 2016-11-03T00:58:54Z

Also update DEFAULT_CONFIG

YuichiNukiyama · 2016-11-03T01:41:51Z

@nchen63 I fixed configration.ts. This commit pass ci. But somewhat there is no self-confidence.(I can't test on local.)

remove old rule label-undefined

nchen63

looks good. Thanks!

YuichiNukiyama · 2016-11-03T04:03:03Z

Thank you for your help 😊

nchen63 · 2016-11-03T13:57:04Z

You're welcome. Thanks for the contribution!

adidahiya · 2016-11-03T14:12:56Z

src/rules/noDuplicateKeyRule.ts

@@ -31,6 +31,7 @@ export class Rule extends Lint.Rules.AbstractRule {
        options: null,
        optionExamples: ["true"],
        type: "functionality",
+        typescriptOnly: false,


This rule was deleted on the master branch because it is covered by the compiler. I'm going to delete it again; I don't want to maintain it just for JS files.

this was removed in next, but not in master

New feature: linting js files

814c490

Merge remote-tracking branch 'tslintupstream/master' into lint_js

160e3fa

jkillian added the Status: In Discussion label Aug 29, 2016

YuichiNukiyama added 2 commits September 3, 2016 22:46

use metadata.typscriptonly

190ae01

test all rules

4500237

Sorry for my large commit.

YuichiNukiyama added 3 commits September 10, 2016 11:15

Merge remote-tracking branch 'tslintupstream/master' into lint_js

39ddc6a

add recommendation

e9b3557

lint js

0ee8c4a

jkillian added PR: waiting for reviewer and removed Status: In Discussion labels Sep 14, 2016

YuichiNukiyama added 2 commits October 29, 2016 09:46

merge master

2c44d39

delete files which was deleted in master

7ae49cb

fix tweak

7eb4118

nchen63 reviewed Oct 29, 2016

View reviewed changes

YuichiNukiyama added 2 commits October 30, 2016 17:23

commentFormatRule

0c97490

arrow-parens

0f4dd24

no-require-import

0b4177e

nchen63 reviewed Oct 31, 2016

View reviewed changes

YuichiNukiyama added 2 commits November 1, 2016 23:44

change recomennded.ts

938cdc4

resolve conflict

b06f12e

nchen63 added PR: waiting for author and removed PR: waiting for reviewer labels Nov 3, 2016

change configration

30929bf

Noah Chen added 4 commits November 2, 2016 22:42

combinedConfig.rules -> combinedConfig.jsRules

8ed9d60

remove old rule label-undefined

Merge branch 'master' into lint_js

d6aaee0

fix lint issues

554b674

add jsRules tests for extend

fb5dd67

nchen63 approved these changes Nov 3, 2016

View reviewed changes

nchen63 merged commit fd5ae5a into palantir:master Nov 3, 2016

YuichiNukiyama deleted the lint_js branch November 3, 2016 04:01

adidahiya removed the PR: waiting for author label Nov 3, 2016

adidahiya reviewed Nov 3, 2016

View reviewed changes

adidahiya mentioned this pull request Nov 3, 2016

Remove no-duplicate-key rule again #1671

Merged

1 task

New feature: lint js files #1515

New feature: lint js files #1515

Conversation

YuichiNukiyama commented Aug 25, 2016 • edited Loading

Related issue

Usage

Good point

Bad point

Note

palantirtech commented Aug 25, 2016

jkillian commented Aug 29, 2016

YuichiNukiyama commented Aug 30, 2016

jkillian commented Sep 1, 2016

YuichiNukiyama commented Sep 1, 2016

jkillian commented Sep 2, 2016

YuichiNukiyama commented Sep 4, 2016

YuichiNukiyama commented Sep 10, 2016

jkillian commented Sep 16, 2016

adidahiya commented Oct 27, 2016

YuichiNukiyama commented Oct 29, 2016

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

nchen63 Oct 29, 2016 • 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

YuichiNukiyama commented Oct 29, 2016

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

nchen63 commented Nov 3, 2016

nchen63 commented Nov 3, 2016

YuichiNukiyama commented Nov 3, 2016

nchen63 left a comment

Choose a reason for hiding this comment

YuichiNukiyama commented Nov 3, 2016

nchen63 commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuichiNukiyama commented Aug 25, 2016 •

edited

Loading

nchen63 Oct 29, 2016 •

edited

Loading