Support yaml configuration (#1598) #3433

jhanschoo · 2017-11-01T09:31:06Z

PR checklist

Addresses an existing issue: Support yaml configuration #1598
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Implement support for yaml. The "js-yaml" library is already being used in documentation, so it's a simple matter of moving a devDependency to a dependency. I know there's talk about using cosmicConfig, but that would require rewriting a number of tests, and this was way simpler haha

Is there anything you'd like reviewers to focus on?

Let me know what other tests I should write.

CHANGELOG.md entry:

[api] Support for YAML config files.

palantirtech · 2017-11-01T09:31:07Z

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

ajafff

Overall LGTM

I left some suggestions.

ajafff · 2017-11-01T19:02:08Z

src/configuration.ts

+// As per eslint convention, yaml config is used if present
+// over json config
+export const JSON_CONFIG_FILENAME = "tslint.json";
+export const CONFIG_FILENAMES = ["tslint.yaml", "tslint.yml", JSON_CONFIG_FILENAME];


consider moving the json file to the first position to avoid unnecessary file lookups

I don't think we need to adopt the behavior of ESLint

ajafff · 2017-11-01T19:10:22Z

src/configuration.ts

+            // TODO: remove in v6.0.0
+            // Try reading in the entire directory and looking for a file with different casing.
+            const filenameLower = filename.toLowerCase();
+            const result = fs.readdirSync(cwd).find((entry) => entry.toLowerCase() === filenameLower);


this reads the directory 3 times, each time only trying to match one file name.
instead it should read the directory once and compare to all possible file names

ajafff · 2017-11-01T19:13:23Z

src/configuration.ts

-            const fileContent = stripComments(fs.readFileSync(resolvedConfigFilePath)
-                .toString()
-                .replace(/^\uFEFF/, ""));
+        if (resolvedConfigFileExt.match(/\.(json|ya?ml)/) !== null) {


prefer /^\.(json|ya?ml)$/.test(resolvedConfigFileExt)

ajafff · 2017-11-01T19:14:52Z

src/configuration.ts

-                rawConfigFile = JSON.parse(fileContent) as RawConfigFile;
+                if (resolvedConfigFileExt === ".json") {
+                    rawConfigFile = JSON.parse(stripComments(fileContent)) as RawConfigFile;
+                } else if (resolvedConfigFileExt.match(/ya?ml/) !== null) {


this condition will always be true. you can remove the condition

ajafff · 2017-11-01T19:16:15Z

src/configuration.ts

+                    }) as RawConfigFile;
+                } else {
+                    // throw error for static analysis purpose; should not happen
+                    throw new Error("File format not supported yet.");


this is dead code

ajafff · 2017-11-01T19:24:49Z

package.json

@@ -39,10 +39,11 @@
  "dependencies": {
    "babel-code-frame": "^6.22.0",
    "builtin-modules": "^1.1.1",
-    "chalk": "^2.1.0",
+    "chalk": "~2.1.0",


I'll look into that and open a separate PR. The change shouldn't be necesary if you use yarn instead of npm

ajafff · 2017-11-01T19:29:47Z

src/configuration.ts

-            showWarningOnce(`Using mixed case tslint.json is deprecated. Found: ${path.join(cwd, result)}`);
+            // TODO: remove in v6.0.0
+            // Try reading in the entire directory and looking for a file with different casing.
+            const filenameLower = filename.toLowerCase();


you can probably remove this line while you are here. we already know filename is lowercase

ajafff · 2017-11-01T19:34:51Z

Personally I'd prefer tslint.json having priority over tslint.ya?ml: json > yaml > yml

@adidahiya want to share your opinion on this?

jhanschoo · 2017-11-01T20:28:54Z

Ok thanks! I'll look into incorporating them in the weekend.

ajafff · 2017-11-05T19:40:02Z

src/configuration.ts

+        for (const filename of filenames) {
+            const index = dirFiles.indexOf(filename);
+            if (index > -1) {
+                return dirFiles[index];


or just return filename;

ajafff · 2017-11-05T19:44:54Z

src/configuration.ts

-                .toString()
-                .replace(/^\uFEFF/, ""));
+        if (/\.(json|ya?ml)/.test(resolvedConfigFileExt)) {
+            const fileContent = fs.readFileSync(resolvedConfigFilePath).toString().replace(/^\uFEFF/, "");


while you are already touching this code: if you pass "utf8" as second argument to fs.readFileSync, you can simply remove the .toString() call

ajafff · 2017-11-05T19:46:51Z

test/executable/executableTests.ts

+                assert.isNotNull(err, "process should exit with error");
+                assert.strictEqual(err.code, 1, "error code should be 1");
+
+                assert.include(stderr, "Failed to load", "stderr should contain notification about failing to load json");


s/json/config/

ajafff · 2017-11-05T19:49:58Z

package.json

+    "license": "Apache-2.0",
+    "engines": {
+        "node": ">=4.8.0"
+    }


please revert the style-only changes in this file

ajafff · 2017-11-05T19:54:12Z

src/configuration.ts

-        if (fs.existsSync(path.join(cwd, filename))) {
-            return filename;
+        const dirFiles = fs.readdirSync(cwd);
+        for (const filename of filenames) {


this might be unexpected as it prefers tslint.yaml over Tslint.json

adidahiya

overall the approach here seems reasonable and is a minimal change 👍 looks like js-yaml is a relatively light dependency, so i'm cool with taking it now

adidahiya · 2017-11-07T19:48:34Z

docs/usage/configuration/index.md

@@ -4,12 +4,12 @@ title: Configuring TSLint
 permalink: /usage/configuration/
 ---

-### tslint.json
+### The `tslint` File


Please just name this "TSLint Configuration"

adidahiya · 2017-11-07T19:51:44Z

src/configuration.ts

-                rawConfigFile = JSON.parse(fileContent) as RawConfigFile;
+                if (resolvedConfigFileExt === ".json") {
+                    rawConfigFile = JSON.parse(stripComments(fileContent)) as RawConfigFile;
+                } else { // /\.ya?ml/.test(resolvedConfigFileExt) === true


what's this comment doing here? if it's stray code, please delete. if it's explanatory, write some additional words at the beginning (don't start with "/") and put it on its own line

jhanschoo · 2017-11-07T19:59:58Z

Alright give me a few mins. Should I squash my commits?

ajafff · 2017-11-07T20:05:12Z

@jhanschoo no need to sqash, we do it anyway while merging

jhanschoo · 2017-11-07T20:05:51Z

Ok thanks!

ajafff · 2017-11-08T18:41:28Z

Thanks @jhanschoo

jhanschoo · 2017-11-09T15:46:57Z

Thanks!

ajafff · 2017-11-09T18:11:47Z

src/configuration.ts

@@ -63,7 +64,10 @@ export interface IConfigurationLoadResult {
    results?: IConfigurationFile;
 }

-export const CONFIG_FILENAME = "tslint.json";


this could actually break some users of our API:

import * as Lint from "tslint"; Lint.Configuration.CONFIG_FILENAME; // no longer valid

I doubt this is used in real world code, so I don't know if this is worth fixing.

Restores the removed export `CONFIG_FILENAME` to avoid breaking API users. Ref: #3433 (comment) [no-log]

Restores the removed export `CONFIG_FILENAME` to avoid breaking API users. Ref: palantir#3433 (comment) [no-log]

jhanschoo force-pushed the master branch from 8233637 to 833ccc7 Compare November 1, 2017 10:03

Support yaml configuration (palantir#1598)

dcf5c55

jhanschoo force-pushed the master branch from 833ccc7 to dcf5c55 Compare November 1, 2017 10:33

ajafff reviewed Nov 1, 2017

View reviewed changes

ajafff reviewed Nov 5, 2017

View reviewed changes

jhanschoo added 2 commits November 6, 2017 09:00

incorporate feedback from @ajafff on issue (palantir#1598)

71504fc

incorporate feedback pt. 2 from @ajafff on issue (palantir#1598)

e3bb4c5

jhanschoo force-pushed the master branch from f786ea1 to e3bb4c5 Compare November 6, 2017 04:16

ajafff approved these changes Nov 6, 2017

View reviewed changes

adidahiya reviewed Nov 7, 2017

View reviewed changes

incorporate feedback from @adidahiya on PR for issue (palantir#1598)

fdaa6db

ajafff merged commit 6d3f78e into palantir:master Nov 8, 2017

ajafff mentioned this pull request Nov 8, 2017

Support yaml configuration #1598

Closed

ajafff reviewed Nov 9, 2017

View reviewed changes

ajafff mentioned this pull request Nov 11, 2017

revert breaking change introduced in #3433 #3477

Merged

4 tasks

adidahiya pushed a commit that referenced this pull request Nov 28, 2017

revert breaking change introduced in #3433 (#3477)

0b3da72

Restores the removed export `CONFIG_FILENAME` to avoid breaking API users. Ref: #3433 (comment) [no-log]

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

Support yaml configuration (palantir#1598) (palantir#3433)

adb5ac9

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

revert breaking change introduced in palantir#3433 (palantir#3477)

3f6b9c3

Restores the removed export `CONFIG_FILENAME` to avoid breaking API users. Ref: palantir#3433 (comment) [no-log]

zhuravlikjb mentioned this pull request Jul 16, 2018

Add eslint/tslint yaml file masks for corresponding schemas SchemaStore/schemastore#494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support yaml configuration (#1598) #3433

Support yaml configuration (#1598) #3433

jhanschoo commented Nov 1, 2017 •

edited

Loading

palantirtech commented Nov 1, 2017

ajafff left a comment

ajafff Nov 1, 2017

ajafff Nov 1, 2017

ajafff Nov 1, 2017

ajafff Nov 1, 2017

ajafff Nov 1, 2017

ajafff Nov 1, 2017

ajafff Nov 1, 2017

ajafff commented Nov 1, 2017

jhanschoo commented Nov 1, 2017

ajafff Nov 5, 2017

ajafff Nov 5, 2017

ajafff Nov 5, 2017

ajafff Nov 5, 2017

ajafff Nov 5, 2017

adidahiya left a comment

adidahiya Nov 7, 2017

adidahiya Nov 7, 2017

jhanschoo commented Nov 7, 2017

ajafff commented Nov 7, 2017

jhanschoo commented Nov 7, 2017

ajafff commented Nov 8, 2017

jhanschoo commented Nov 9, 2017

ajafff Nov 9, 2017

Support yaml configuration (#1598) #3433

Support yaml configuration (#1598) #3433

Conversation

jhanschoo commented Nov 1, 2017 • edited Loading

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech commented Nov 1, 2017

ajafff left a 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

Choose a reason for hiding this comment

ajafff commented Nov 1, 2017

jhanschoo commented Nov 1, 2017

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

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhanschoo commented Nov 7, 2017

ajafff commented Nov 7, 2017

jhanschoo commented Nov 7, 2017

ajafff commented Nov 8, 2017

jhanschoo commented Nov 9, 2017

Choose a reason for hiding this comment

jhanschoo commented Nov 1, 2017 •

edited

Loading