Respect tslint configs hierarchical order #214

konstantinov90 · 2019-02-16T14:39:55Z

Hi!

The "why" for this PR is in the README.md, please consider merging this request!

johnnyreilly · 2019-02-16T15:09:09Z

~~Could you elaborate please?~~

Sorry - I see what you mean! I've left some comments

johnnyreilly · 2019-02-16T15:10:50Z

src/ApiIncrementalChecker.ts

@@ -90,7 +117,7 @@ export class ApiIncrementalChecker implements IncrementalCheckerInterface {
  }

  public hasLinter(): boolean {
-    return !!this.linterConfig;
+    return true;


This looks incorrect; not everyone wants to use tslint. Could you amend this please?

johnnyreilly · 2019-02-16T15:18:12Z

src/ApiIncrementalChecker.ts

+        linter.lint(
+          updatedFile,
+          undefined!,
+          this.linterConfig || this.getLinterConfig(updatedFile)


Can you talk me through this line please? What is this.linterConfig in this context? What would cause it to be false-y / when would you expect to call this.getLinterConfig(updatedFile)

I tried to be more explicit this time around, but still: this.linterConfig - is a TSLint config, which is defined by path, provided by user in webpack config. Conversely, when user provides true instead of config-file path, this.linterConfig should be undefined, instead, it will be calculated inside this.getLinterConfig

konstantinov90 · 2019-02-16T15:21:44Z

The "why" for this PR is in the README.md, please consider merging this request!

Could you elaborate please?

Suppose I have a project with three folders

tslint.json
src/
    folder1/
        ...files...
    folder2/
        ...files...
    folder3/
        ...files...
        tslint.json

content of ./tslint.json

{
"extends": [  "tslint-react"  ],
"rules": {
   "no-any": true
 }
}

content of src/folder3/tslint.json

{
   extends: ["../../tslint.json"],
  "rules": {
        "no-any": false
   }
}

So with such a configuration I say that I want to use any inside src/folder3. And running tslint -p . shows that tslint indeed respects this overridden rule "no-any". Same goes to my IDE, it does respect this rule. And the only piece of puzzle is this great plugin, which, unfortunately, ignores "no-any": false inside folder3, because it assumes that only one config file exists. And this is what I am trying to fix

johnnyreilly · 2019-02-16T15:33:51Z

Cool - could you take a look at my review comments please?

konstantinov90 · 2019-02-16T15:35:10Z

Cool - could you take a look at my review comments please?

yeah, working on it)

konstantinov90 · 2019-02-16T16:41:25Z

/fixed

johnnyreilly · 2019-02-16T17:01:55Z

Great - thanks! Could you write a test please? Integration makes the most sense I think

konstantinov90 · 2019-02-16T17:02:25Z

Great - thanks! Could you write a test please? Integration makes the most sense I think

will do

konstantinov90 · 2019-02-16T18:43:46Z

/test done

johnnyreilly · 2019-02-20T07:00:35Z

Could you catch up your fork with the changes in master please? We've just pushed out a new version which affects this fork

…nfigs-hierarchical-order

konstantinov90 · 2019-02-20T08:04:05Z

Could you catch up your fork with the changes in master please? We've just pushed out a new version which affects this fork

done

johnnyreilly · 2019-02-21T05:31:12Z

src/ApiIncrementalChecker.ts

+      }
+      this.linterConfigs[dirname] = config;
+    } else {
+      if (dirname === '.') {


can you talk me through what happens in this branch please? I've read it a bunch of times but I'm not so clear how this copes with:

if there's only a tslint.json file in a particular directory of the application but not in the root

if this genuinely manages to walk up the directory tree. When this.linterConfigs[dirname] = this.getLinterConfig(dirname); is invoked doesn't it stick with the current directory rather than walk up to the parent? Maybe I'm misunderstanding this...

Do you mean "What if I want to subject to tslint some particular parts of my project by providing tslint.config inside, for example, src/components folder, but not in root folder"? I assumed that it is not a legitimate case, because of the chains of imports inside src/components that could lead to files from the root of project, that is why I throw an error, correct me if I'm wrong

Follow these steps:

first file to be processed is src/components/SomeComponent.tsx. On the first run of getLinterConfig we get const dirname = 'src/components'

check if src/components/tslint.json exists (suppose it does not)

invoke second run of getLinterConfig('src/components')

const dirname = path.dirname('src/components') = 'src'

check src/tslint.json, if it does, then OK, we've got our config, if not - then invoke third run of getLinterConfig('src')

check ./tslint.json, if it does not exist - then throw an error

Cool - that's clear thanks

I assumed that it is not a legitimate case, because of the chains of imports inside src/components that could lead to files from the root of project, that is why I throw an error, correct me if I'm wrong

I'm not sure it is an error... How does tslint itself behave in those circumstances? My assumption is that in this case we should just not lint...

yeah, you're right, TSLint CLI warns No valid rules have been specified for TypeScript files for root file, but still check inner files. Fixin' it

johnnyreilly · 2019-02-22T08:45:12Z

src/ApiIncrementalChecker.ts

@@ -115,10 +145,6 @@ export class ApiIncrementalChecker implements IncrementalCheckerInterface {
  }

  public getLints(_cancellationToken: CancellationToken) {
-    if (!this.linterConfig) {


Does the removal of this speedy return mean that users who are not using tslint pay a performance tax as we still check for tslint.jsons?

If so I'd like to try and avoid this

as far as I can see, the answer is no, because getLints method get called only if hasLinter returns true (check out run function in src/service.ts)

but still, I guess it never hurts to be more explicit. Fixed it

So it looks like you're right - it does appear to be unnecessary. You had it right first time. 😁

Could you undo that - there's no point checking something a second time when it's already been checked.

I think then we'll be good to merge. Thanks for all your work!

you got it!

johnnyreilly

Actually one last thing: it looks like getLinterConfig is repeated in src/IncrementalChecker.ts and src/ApiIncrementalChecker.ts.

Is there a way we could convert this into a shared function both modules could use please? You'd probably need to supply config as a parameter for instance.

konstantinov90 · 2019-02-23T09:34:22Z

@johnnyreilly how about that?

johnnyreilly · 2019-02-23T09:50:11Z

So I have bad news @konstantinov90. I'm a man with prejudices. Or let's call them "opinions" 😉

I don't find inheritance the most joyful thing to work with in a codebase. Whilst I completely acknowledge it could be used to solve this problem I'm afraid my innate dislike of inheritance means it troubles me deeply.

If you could indulge my preferences and go for an approach that doesn't involve inheritance I would greatly appreciate it 😏

konstantinov90 · 2019-02-23T10:49:37Z

well then, I see two other options

composition, instead of inheritance, but it will possibly require not-so-minor refactoring (I mean create some helper class LinterConfigManager, which would incorporate singleFixedConfig or linterConfigs for the whole project filetree, linterExclusions and provide a method getConfigForFile(filepath), which would be called from within getLints method)
brutally extract function from both IncrementalChecker classes, but such a function would not be pure function, because (as it is now) it modifies this.linterConfigs and this.linterExclusions. Theoretically, this function could be more pure, if it would provide just configPath, but this would mean that we simply split the complexity of this function in two parts, and one part would still be the method of both IncrementalChecker classes.

What do you prefer?

johnnyreilly · 2019-02-23T11:00:37Z

brutally extract function from both IncrementalChecker classes, but such a function would not be pure function, because (as it is now) it modifies this.linterConfigs and this.linterExclusions. Theoretically, this function could be more pure, if it would provide just configPath, but this would mean that we simply split the complexity of this function in two parts, and one part would still be the method of both IncrementalChecker classes.

I prefer this but I don't think it need be brutal. Create a shared makeGetLinterConfig function with parameters of linterConfigs / linterExclusions / whatever else is required. This function returns a getLinterConfig function and can operate on the supplied parameters to makeGetLinterConfig in closure scope.

Sound good?

konstantinov90 · 2019-02-23T12:45:21Z

/done

johnnyreilly · 2019-02-23T13:30:46Z

Should ship with https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v1.0.0-alpha.8

konstantinov90 · 2019-02-23T14:43:02Z

Should ship with https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v1.0.0-alpha.8

great, thanks!

johnnyreilly · 2019-02-23T15:04:24Z

Thanks for your work!

Alexander Konstantinov added 5 commits February 16, 2019 14:26

respect TSLint configs hierarchical order

46e2c68

fixed settings + doc

19b8d29

pass correct options to IncrementalChecker

62ec738

fix passing settings

f983714

fix tests

19350d1

johnnyreilly reviewed Feb 16, 2019

View reviewed changes

review fix #1

5e91fbe

Alexander Konstantinov added 3 commits February 16, 2019 21:33

integration test for tslint hierarchical order

35e3663

fix quotes

0c569f1

moved test to common

280a8e3

actual test files...

390582b

johnnyreilly mentioned this pull request Feb 16, 2019

ignoreLintWarnings option #213

Merged

Alexander Konstantinov added 2 commits February 20, 2019 10:04

Merge remote-tracking branch 'upstream/master' into respect-tslint-co…

0d39337

…nfigs-hierarchical-order

changelog

68d5159

johnnyreilly reviewed Feb 21, 2019

View reviewed changes

Alexander Konstantinov added 3 commits February 22, 2019 00:37

review related fix

5672a4a

fix test after prettier

6652bf0

fix

686f61c

johnnyreilly reviewed Feb 22, 2019

View reviewed changes

johnnyreilly reviewed Feb 23, 2019

View reviewed changes

optimize common functionality

461843e

johnnyreilly merged commit 03c612e into TypeStrong:master Feb 23, 2019

konstantinov90 deleted the respect-tslint-configs-hierarchical-order branch February 23, 2019 14:42

johnnyreilly mentioned this pull request Mar 7, 2019

TypeError: Cannot read property 'context' of undefined #118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect tslint configs hierarchical order #214

Respect tslint configs hierarchical order #214

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 16, 2019 •

edited

Loading

johnnyreilly Feb 16, 2019

konstantinov90 Feb 16, 2019

johnnyreilly Feb 16, 2019

konstantinov90 Feb 16, 2019

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 16, 2019 •

edited

Loading

konstantinov90 commented Feb 16, 2019

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 16, 2019

konstantinov90 commented Feb 16, 2019

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 20, 2019

konstantinov90 commented Feb 20, 2019

johnnyreilly Feb 21, 2019

konstantinov90 Feb 21, 2019

johnnyreilly Feb 21, 2019

konstantinov90 Feb 21, 2019

johnnyreilly Feb 22, 2019

konstantinov90 Feb 22, 2019 •

edited

Loading

konstantinov90 Feb 22, 2019

johnnyreilly Feb 22, 2019

konstantinov90 Feb 23, 2019

johnnyreilly left a comment

konstantinov90 commented Feb 23, 2019

johnnyreilly commented Feb 23, 2019

konstantinov90 commented Feb 23, 2019 •

edited

Loading

johnnyreilly commented Feb 23, 2019

konstantinov90 commented Feb 23, 2019

johnnyreilly commented Feb 23, 2019

konstantinov90 commented Feb 23, 2019

johnnyreilly commented Feb 23, 2019

Respect tslint configs hierarchical order #214

Respect tslint configs hierarchical order #214

Conversation

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 16, 2019 • 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

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 16, 2019 • edited Loading

konstantinov90 commented Feb 16, 2019

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 16, 2019

konstantinov90 commented Feb 16, 2019

konstantinov90 commented Feb 16, 2019

johnnyreilly commented Feb 20, 2019

konstantinov90 commented Feb 20, 2019

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

konstantinov90 Feb 22, 2019 • 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

johnnyreilly left a comment

Choose a reason for hiding this comment

konstantinov90 commented Feb 23, 2019

johnnyreilly commented Feb 23, 2019

konstantinov90 commented Feb 23, 2019 • edited Loading

johnnyreilly commented Feb 23, 2019

konstantinov90 commented Feb 23, 2019

johnnyreilly commented Feb 23, 2019

konstantinov90 commented Feb 23, 2019

johnnyreilly commented Feb 23, 2019

johnnyreilly commented Feb 16, 2019 •

edited

Loading

johnnyreilly commented Feb 16, 2019 •

edited

Loading

konstantinov90 Feb 22, 2019 •

edited

Loading

konstantinov90 commented Feb 23, 2019 •

edited

Loading