Add exclude section to configuration files. #2409

mmkal · 2017-03-25T22:45:59Z

This allows users to define default cli options in tslint.json.

PR checklist

Addresses Feature: ".tslintignore" file to exclude globs #73
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Adds cliOptions object to IConfigurationFile interface in src/configuration.ts.
Modifies src/cli.ts to read from the configuration file (passed in via cli or default tslint.json) and use configuration.cliOptions as default values for the command line options.

This would, for example, allow exluding files from linting (the topic of the issue) like this:

{
  "extends": "tslint:latest",
  "cliOptions": {
      "exclude": [
          "bin",
          "lib/*generated.js"
      ]
  }
}

suggested tags: [enhancement]

This allows users to define default cli options in tslint.json. e.g. if you want to exclude the "bin" directory from linting, you could add this to your tslint.json: "cliOptions": { "exclude": "bin" }

palantirtech · 2017-03-25T22:46:00Z

Thanks for your interest in palantir/tslint, @mmkal! 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.

adidahiya · 2017-03-27T13:52:52Z

Cool, overall this seems reasonable given all the user feedback around CLI options (mainly summed up in #2227). It does represent a change of philosophy in what tslint.json does. Previously, the configuration file could only specify how to lint, but now it has the ability to specify what to lint as well. See #73 (comment) and #629 (comment). What do you think @jkillian @nchen63 @ajafff @andy-hanson @andy-ms @mgechev @ChrisMBarr? Any objections?

adidahiya

Now for PR feedback:

I don't think this exactly solves Feature: ".tslintignore" file to exclude globs #73 because tslint doesn't look for additional tslint.json files after it has found a relevant one it can use to lint. I believe the request for a .tslintignore file was made so that users could have multiple such ignore files across their codebase and a single tslint.json (I'm not 100% sure though, you should go back to that thread and confirm).
we should name the top-level config key "linterOptions", like this private API we use for testing.
there are many more CLI options, not just exclude. we need tests for those too. in particular we need to test things like path resolution
rulesDirectory is currently handled specially as a top-level tslint.json key. It should probably move into linterOptions to be consistent (yeah it's a breaking change but we're on the 5.0 track now so we can do that).

ajafff · 2017-03-27T14:09:13Z

I'm ok with specifying what to lint and making all CLI options available in tslint.json.

There are some questions that need to be resolved:

move all options to linterOptions as @adidahiya suggests OR make them all top level options? Since linterOptions.typeCheck is private, the latter would not break any user
add a new config include to be consistent with tsc?
should entries in exclude be merged when extending configurations?
do options passed via CLI merge or override options in the config?

adidahiya · 2017-03-27T14:22:24Z

move all options to linterOptions as @adidahiya suggests OR make them all top level options?

not all options are configurable via the CLI. for example, "extends" and "rules". So I would prefer to group all the CLI options together into one object and not make them top-level options. semantically, "extends" should always be a top-level field. and I wouldn't want to break backcompat just yet by forcing nesting of any existing top-level option (rules, rulesDirectory).

add a new config include to be consistent with tsc?

yeah, I would accept that as a PR if it had the same semantics as tsc. but I wouldn't block this PR on it.

should entries in exclude be merged when extending configurations?

hmm, not sure about this one. In general, we don't expect base/intermediate configurations (those you are extending) to use "exclude" very much (it's just harder to reason about).

do options passed via CLI merge or override options in the config?

I think they should override the config file's options (this is how most CLIs work)

mmkal · 2017-03-27T20:07:39Z

I don't think this exactly solves #73 because tslint doesn't look for additional tslint.json files after it has found a relevant one it can use to lint. I believe the request for a .tslintignore file was made so that users could have multiple such ignore files across their codebase and a single tslint.json (I'm not 100% sure though, you should go back to that thread and confirm).

I didn't go the .tslintignore route because it felt clunkier and more obscure. It feels much cleaner to me to be able to open up the tslint.json file and from that be able to understand what running tslint is going to do. But you're right it doesn't solve it precisely. Maybe if this gets in we could see if anyone thinks .tslintignore is still worth developing. I suspect this will cover almost all cases.

we should name the top-level config key "linterOptions", like this private API we use for testing.

Sure, will push an update soon.

there are many more CLI options, not just exclude. we need tests for those too. in particular we need to test things like path resolution

I'm unsure what you mean by this. In terms of testing the code I'm adding, I'm almost exclusively relying on existing functionality.

In terms of implementation, I'm loading the configuration object using findConfigurationPath and loadConfigurationFromPath in src/configuration.ts. Then using those values as defaults using optimist.default(...). Is it necessary to test that part? My feeling there was that it would be simply re-testing that optimist.default(...) works correctly. Or did you mean I should add some more tests of the configuration merging?

I didn't find any existing tests of src/cli.ts, but I could add a new test file?

rulesDirectory is currently handled specially as a top-level tslint.json key. It should probably move into linterOptions to be consistent (yeah it's a breaking change but we're on the 5.0 track now so we can do that).

Sure - should I deprecate the existing rulesDirectory? If so is there an example of something similar being done?

should entries in exclude be merged when extending configurations?

hmm, not sure about this one. In general, we don't expect base/intermediate configurations (those you are extending) to use "exclude" very much (it's just harder to reason about).

I wasn't sure either. But I had a vision of working on a team project and extending an existing tslint.json and tearing my hair out trying to figure out why some of my files were being excluded from linting so I made it replace existing values instead.

do options passed via CLI merge or override options in the config?

I think they should override the config file's options (this is how most CLIs work)

Yeah, I was basing this on how tsc works. You can set sourceMap: false in tsconfig.json but override it with tsc --sourceMap -p . on the cli for local builds etc.

jkillian · 2017-03-28T03:29:17Z

Haven't reviewed the actual code changes, but I'm fine w/ this in principle if it makes things easier for users. There are some questions about how this affects integration with other tools though:

In the past it was up to the integrating tool to decide what files to lint. If you were using an IDE, your IDE would have to tell TSLint which files to lint. If you're using webpack, this would be controlled by your webpack rule/loader test field. If using the CLI, it would be controlled by the command line options you pass to TSLint.

In this PR, presumably the proposed cliOptions field would only affect the CLI? Or would IDEs be expected to read/parse it as well? What about a webpack loader?

adidahiya · 2017-03-28T04:57:11Z

@mmkal

It feels much cleaner to me to be able to open up the tslint.json file and from that be able to understand what running tslint is going to do. But you're right it doesn't solve it precisely. Maybe if this gets in we could see if anyone thinks .tslintignore is still worth developing. I suspect this will cover almost all cases.

I agree, this is better than .tslintignore.

I didn't find any existing tests of src/cli.ts, but I could add a new test file?

Yeah, never mind the path resolution thing; I just think there should just be a test that verifies that CLI options can override linterOptions in the config file by testing the whole workflow with source file fixtures. Add a sample project to lint as a test fixture and run the CLI in a way that checks the new features work as intended.

should I deprecate the existing rulesDirectory?

Yeah, let's deprecate it with a console.warn message directing users to move the option into a linterOptions block, but don't remove the feature completely.

I made it replace existing values instead.

Cool, this makes sense to implement now and wait for feedback if the behavior should be changed later

adidahiya · 2017-03-28T05:01:02Z

@jkillian

In this PR, presumably the proposed cliOptions field would only affect the CLI? Or would IDEs be expected to read/parse it as well? What about a webpack loader?

Yes, I would actually expect IDEs and webpack loaders to read these config options (eventually). With tsc, we've found that configuring compiler options in one place, declaratively, works well.

adidahiya · 2017-03-28T05:18:37Z

@mmkal ah, actually we do have CLI tests, they're in test/executable/executableTests.ts

jkillian · 2017-03-28T14:40:58Z

Yes, I would actually expect IDEs and webpack loaders to read these config options (eventually). With tsc, we've found that configuring compiler options in one place, declaratively, works well.

Good point and agreed, tsconfig.json does work out quite nicely. Since other tools may be reading this data, it also supports your point that we shouldn't call the key cliOptions. What we should do, then is have good utility functions so that 3rd-party tool integration is easy. Nobody will want to have to figure out how to calculate the correct combination of excludes and includes and globs themselves! We'll need to make sure this is a stable API that 3rd-party tools can rely upon.

mmkal · 2017-04-26T04:43:19Z

@adidahiya just to give an update on this... I haven't had a lot of time to look at this since we last spoke, but I managed to look at it a little over the weekend. Moving cliOptions to linterOptions is a little more complicated than I'd originally thought. There are a couple of design decisions to consider, like

Should I camelCase the options passed in?
If so, should I allow non-camel-cased options?

Camel casing is more consistent with the rest of tslint.json, but it increases the complexity because the cli options are hyphenated.

The deprecation of rulesDir and linterOptions.typeCheck is also a little unclear. Presumably I need to support the merging of rules directories from parent configs? If so, how do I reconcile this with the decision above, to overwrite rather than replace properties? Different behavior for different properties? And since you've suggested I deprecate rulesDir, this would mean changing it in a lot of different files since it wouldn't be good to have tslint itself using a deprecated feature.

Another problem I've seen is that I can't actually seem to make the executableTests run. npm test and npm run verify seem to ignore them.

Taking all this into account, my solution is no longer looking like a cheap win. As I don't know the codebase well, I would rather not be making big sweeping changes. So I can make three suggestions for what I can do next:

Go back to using cliOptions, but don't include rules-dir as that's already implemented.
Keep plugging away at linterOptions, after getting answers to the questions above (my preference would be support camelCase and non-camel-case by transforming from one to the other, so that the current implementation of typeCheck can be left alone. This also makes documentation easier as it may be good enough to say "use any cli options in the linterOptions object"). I would still want to exclude rules-dir here to avoid dealing with any complications over inheriting from other configs. If you want to move rules-dir inside linterOptions, it'd have to be done by someone who can give up a bit more time on this project.
Reduce the scope of this work to what it was originally about, and only add the "exclude" property to linterOptions. This conveniently avoids the camelCase problem too. Everything else could then be left more or less as it is.

I'd be ok with any one of those three - or any other suggestion you might have. As long as they allow me to only make small code changes - otherwise I won't realistically be able to get them done.

adidahiya · 2017-05-24T02:50:40Z

@mmkal sorry for the delay. Let's just go with the easiest approach, (3), limiting the scope of the work in this PR.

This reverts commit 4449db0.

Looking at the project file in the root wasn't sufficient; we need to check if there's an exclude value every time we read a project file. Also make exclude always an array, not a string.

mmkal · 2017-09-03T17:32:58Z

@adidahiya updated - sorry it took so long, I ended up finding a workaround for the project I needed this for, and I didn't have a lot of time to look at this outside of work.

You were right about using __filename not making sense. I moved the check for whether a file is excluded to just before it's linted, since the config for that file is being loaded there already.

misha-kaletsky-zocdoc

Works for me

adidahiya · 2017-09-06T03:11:03Z

@mmkal I pushed a few small tweaks to your branch.

@ajafff mind taking a look here?

mmkal · 2017-09-06T03:21:19Z

whoops, commented from my work account. Your changes look good to me @adidahiya, thanks!

ajafff

Looks good.
Solving the relative vs absolute path issue when matching the glob pattern can be done in a followup.

ajafff · 2017-09-06T17:13:24Z

src/configuration.ts

-        typeCheck?: boolean;
-    };
+    linterOptions?: Partial<{
+        typeCheck: boolean;


you can remove the typeCheck. It's no longer used by the tests

ajafff · 2017-09-06T17:33:36Z

src/runner.ts

+            return false;
+        }
+        const fullPath = path.resolve(filepath);
+        return configFile.linterOptions.exclude.some((pattern) => new Minimatch(pattern).match(fullPath));


The excluded paths should probably be resolved relative to the location of tslint.json.
If /foo/tslint.json excludes baz.ts we should only exclude /foo/baz.ts and still lint /foo/bar/baz.ts.

That will only work when filepath is an absolute path.

ajafff · 2017-09-06T17:35:32Z

test/configurationTests.ts

+            };
+
+            const actualConfig = extendConfigurationFile(baseConfig, extendingConfig);
+            assertConfigEquals(actualConfig, expectedConfig);


out of curiosity: what happens if extendingConfig contains an empty object as "linterOptions"? Would that override the whole object including "exclude"?

No, only if exclude is specified. I added a test for this below.

mmkal

@ajafff I added a test for an extending config with empty linterOptions, and got rid of typeCheck.

mmkal · 2017-09-07T01:39:49Z

test/configurationTests.ts

+            };
+
+            const actualConfig = extendConfigurationFile(baseConfig, extendingConfig);
+            assertConfigEquals(actualConfig, expectedConfig);


No, only if exclude is specified. I added a test for this below.

ghost · 2017-09-07T14:50:02Z

Will this be in 5.8?

ajafff · 2017-09-07T14:51:22Z

@KSuttle-cng Yes, this will be part of the next release

ghost · 2017-09-07T14:57:46Z

Cool, any timeline for this? I've been trying to get the CLI to respect the excludes from the tslint.json, but I have to add --exclude for every entry.

ajafff · 2017-09-07T15:08:08Z

There's no release schedule. Maybe I get around to it during the next week.

ghost · 2017-09-07T15:14:02Z

Awesome. Thanks @ajafff.

Per discussion, we want to be able to keep the samples simple so don't want to force use of const there. Currently, putting `tslint:disable` in all files appears to be the best way to accomplish this. Hopefully once palantir/tslint#2409 lands in the next tslint release we can move over to juts excluding our samples folder from linting.

Add cliOptions section to configuration files.

4449db0

This allows users to define default cli options in tslint.json. e.g. if you want to exclude the "bin" directory from linting, you could add this to your tslint.json: "cliOptions": { "exclude": "bin" }

mmkal mentioned this pull request Mar 25, 2017

Feature: ".tslintignore" file to exclude globs #73

Closed

adidahiya suggested changes Mar 27, 2017

View reviewed changes

adidahiya added the Status: In Discussion label Mar 27, 2017

adidahiya added PR: waiting for author and removed Status: In Discussion labels Mar 28, 2017

adidahiya mentioned this pull request Mar 29, 2017

Warning: unhandled node type IndexSignature #2432

Closed

ajafff mentioned this pull request Apr 11, 2017

No formatter option in tslint.json? #2543

Closed

adidahiya added PR: waiting for reviewer Status: In Discussion and removed PR: waiting for author labels May 8, 2017

adidahiya added PR: waiting for author and removed PR: waiting for reviewer Status: In Discussion labels May 24, 2017

mmkal and others added 4 commits June 10, 2017 15:24

Add exclude to linter options object in IConfigurationFile

e9e5be1

Revert "Add cliOptions section to configuration files."

31a083b

This reverts commit 4449db0.

Merge branch 'master' into json-cli-options

824014a

Merge branch 'master' into json-cli-options

d99a589

mmkal and others added 3 commits September 3, 2017 12:44

refactor: move check for if file is excluded to just before linting

2834735

Looking at the project file in the root wasn't sufficient; we need to check if there's an exclude value every time we read a project file. Also make exclude always an array, not a string.

Merge remote-tracking branch 'palantir/master' into json-cli-options

5ea6f1c

chore: specify encoding explicitly when writing file

d5b1335

mmkal changed the title ~~Add cliOptions section to configuration files.~~ Add exclude section to configuration files. Sep 3, 2017

adidahiya added 2 commits September 5, 2017 20:08

immutable isFileExcluded filter function

bd1a2ec

Fix code docs for IConfigurationFile#linterOptions

beb7fd4

misha-kaletsky-zocdoc approved these changes Sep 6, 2017

View reviewed changes

adidahiya approved these changes Sep 6, 2017

View reviewed changes

adidahiya added this to the TSLint v5.8 milestone Sep 6, 2017

ajafff approved these changes Sep 6, 2017

View reviewed changes

mmkal added 2 commits September 6, 2017 21:06

remove typeCheck from linterOptions

8f11c8c

add test for extending with empty linterOptions

d6982e1

mmkal commented Sep 7, 2017

View reviewed changes

adidahiya merged commit ec9fe9f into palantir:master Sep 7, 2017

adidahiya mentioned this pull request Sep 7, 2017

Inconsistent usage of --exclude option #1560

Closed

ajafff mentioned this pull request Sep 7, 2017

config: use resolved exclude pattern #3205

Merged

4 tasks

lukehoban mentioned this pull request Oct 16, 2017

Remove the comments to disable the linter. pulumi/pulumi-cloud#98

Merged

Vinnl mentioned this pull request Oct 25, 2017

Globs in linterOptions.exclude work different from passing the --exclude argument #3390

Closed

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

Add support for exclude globs in configuration files (palantir#2409)

fd4fd19

mmkal deleted the json-cli-options branch May 23, 2018 12:59

rich-newman mentioned this pull request Oct 10, 2018

Support for "exclude" rich-newman/typescript-analyzer#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exclude section to configuration files. #2409

Add exclude section to configuration files. #2409

mmkal commented Mar 25, 2017 •

edited

Loading

palantirtech commented Mar 25, 2017

adidahiya commented Mar 27, 2017

adidahiya left a comment •

edited

Loading

ajafff commented Mar 27, 2017

adidahiya commented Mar 27, 2017 •

edited

Loading

mmkal commented Mar 27, 2017

jkillian commented Mar 28, 2017

adidahiya commented Mar 28, 2017

adidahiya commented Mar 28, 2017

adidahiya commented Mar 28, 2017

jkillian commented Mar 28, 2017

mmkal commented Apr 26, 2017 •

edited

Loading

adidahiya commented May 24, 2017

mmkal commented Sep 3, 2017

misha-kaletsky-zocdoc left a comment

adidahiya commented Sep 6, 2017

mmkal commented Sep 6, 2017

ajafff left a comment

ajafff Sep 6, 2017

ajafff Sep 6, 2017

ajafff Sep 6, 2017

mmkal Sep 7, 2017

mmkal left a comment

mmkal Sep 7, 2017

ghost commented Sep 7, 2017

ajafff commented Sep 7, 2017

ghost commented Sep 7, 2017

ajafff commented Sep 7, 2017

ghost commented Sep 7, 2017

Add exclude section to configuration files. #2409

Add exclude section to configuration files. #2409

Conversation

mmkal commented Mar 25, 2017 • edited Loading

PR checklist

Overview of change:

palantirtech commented Mar 25, 2017

adidahiya commented Mar 27, 2017

adidahiya left a comment • edited Loading

Choose a reason for hiding this comment

ajafff commented Mar 27, 2017

adidahiya commented Mar 27, 2017 • edited Loading

mmkal commented Mar 27, 2017

jkillian commented Mar 28, 2017

adidahiya commented Mar 28, 2017

adidahiya commented Mar 28, 2017

adidahiya commented Mar 28, 2017

jkillian commented Mar 28, 2017

mmkal commented Apr 26, 2017 • edited Loading

adidahiya commented May 24, 2017

mmkal commented Sep 3, 2017

misha-kaletsky-zocdoc left a comment

Choose a reason for hiding this comment

adidahiya commented Sep 6, 2017

mmkal commented Sep 6, 2017

ajafff left a comment

Choose a reason for hiding this comment

ajafff Sep 6, 2017

Choose a reason for hiding this comment

ajafff Sep 6, 2017

Choose a reason for hiding this comment

ajafff Sep 6, 2017

Choose a reason for hiding this comment

mmkal Sep 7, 2017

Choose a reason for hiding this comment

mmkal left a comment

Choose a reason for hiding this comment

mmkal Sep 7, 2017

Choose a reason for hiding this comment

ghost commented Sep 7, 2017

ajafff commented Sep 7, 2017

ghost commented Sep 7, 2017

ajafff commented Sep 7, 2017

ghost commented Sep 7, 2017

mmkal commented Mar 25, 2017 •

edited

Loading

adidahiya left a comment •

edited

Loading

adidahiya commented Mar 27, 2017 •

edited

Loading

mmkal commented Apr 26, 2017 •

edited

Loading