Add extends support #1016

unional · 2016-03-05T10:15:35Z

I want to add some test in but don't know how. I have added a config.json file. Can you give me some pointers?

Also, please comment on the name and location of the functions.

I can adjust accordingly.

edit from @adidahiya: closes #997

unional · 2016-03-05T10:22:43Z

Will update the PR. It should merge the whole config. Not just the rules.

Do you want it to be done directly in loadConfigurationFromFile()?

unional · 2016-03-06T02:24:21Z

Move extends logic into loadConfigurationFromPath

Test is skipped right now because I don't know how to get the config copied and referenced.

unional · 2016-03-06T02:25:04Z

Currently it should support relative path. Once this is okey from you then I will code for array and package support.

unional · 2016-03-06T07:19:52Z

Updated to support package and array. Definitely need testing

jkillian · 2016-03-07T16:50:28Z

Thanks for the PR @unional! I haven't reviewed the code, but I like the extends syntax, simple and usable

unional · 2016-03-07T17:07:28Z

Please take a look at the PR when you have time. While the test passed, when I try to use it using npm link (with vscode) it is not working. I want to figure out why.

Haven't test directly with tslint cli or gulp-tslint yet.

jkillian · 2016-03-07T23:05:59Z

src/configuration.ts

+    }
+    let parentRules = parentConfig.rules;
+    if (parentRules) {
+        for (let name in parentRules) {


We prefer for (const name of Object.keys(parentRules)) {

unional · 2016-03-07T23:34:00Z

@jkillian , what would you suggest in adding test? You can see in https://github.com/palantir/tslint/pull/1016/files#diff-7adcebcb9ab1532bda827cc5be7bc94bR6, I'm routing back to the test source for the config file.

Any suggestion?

jkillian · 2016-03-07T23:44:06Z

Well, the cwd for the tests should be the root of the project, so instead of doing __dirname + "/../../test/config/tslint-extends-rules.json" you should just be able to use ./test/config/tslint-extends-rules.json.

As far as testing with a package, I think the easiest solution will be to make a super-simple npm package and list it as a devDependency instead of trying to make a fake npm package somewhere.

unional · 2016-03-07T23:44:54Z

Well, the cwd for the tests should be the root of the project, so instead of doing __dirname + "/../../test/config/tslint-extends-rules.json" you should just be able to use ./test/config/tslint-extends-rules.json.

That doesn't work because the config/* is not copied to build folder.

unional · 2016-03-07T23:45:23Z

As far as testing with a package, I think the easiest solution will be to make a super-simple npm package and list it as a devDependency

I have one ready.

jkillian · 2016-03-07T23:50:40Z

That doesn't work because the config/* is not copied to build folder.

But loadConfigurationFromPath uses fs.readFileSync, which if supplied a relative path, will load that path relative to the process's current working directory. In this case, the cwd for running tests should be the project root, and you should be able to use a relative path like ./test/config/tslint-extends-rules.json. Have you tried this and it doesn't work? Perhaps I'm not understanding something correctly here.

unional · 2016-03-07T23:52:27Z

The cwd at that moment is ./build/test/, thus __dirname + "../../test/config/..." point to the right file.

Update: You are correct. it does start at root. 👍

unional · 2016-03-08T00:00:58Z

What's your opinion on delete config.extends?

unional · 2016-03-08T00:06:42Z

Don't have to review code at the moment. Found bugs. Since I can now run tests, I'll get them all work and do a rebase. Will ping you soon. 🌹

jkillian · 2016-03-08T00:21:23Z

What's your opinion on delete config.extends?

Probably better to not do this I'd say

unional · 2016-03-08T00:40:49Z

Alrighty.

unional · 2016-03-08T01:03:37Z

Tests in, passed. Code rebased. Ready to go! @jkillian

unional · 2016-03-08T01:08:26Z

src/configuration.ts

+        let packageJson = JSON.parse(fileData);
+        return path.resolve(`node_modules/${relativeFilePath}/${packageJson.main}`);
+    } else {
+        return path.resolve(relativeTo, relativeFilePath);


Do you want to change this to:

return getRelativePath(relativeFilePath, relativeTo);

Sure, let's use that instead

unional · 2016-03-09T02:29:16Z

With this in, custom rules could be published as tslint-config-X too. Rules directory will be merged.:

// tslint-eslint-rules -> tslint-config-eslint
// main: tslint.json
{
  "rulesDirectory": "./<local rule relative path>",
  "rules": {
    // Can add default rules value here, and
    // disable conflicting core rules if needed.
  } 
}

unional · 2016-03-31T18:29:04Z

Build failing because path.isAbsolute is not available in older node. Any suggestion?

unional · 2016-03-31T18:32:41Z

https://github.com/sindresorhus/path-is-absolute

jkillian · 2016-03-31T20:53:02Z

https://github.com/sindresorhus/path-is-absolute

Feel free to just add that as a dependency and use it. I'm not sure how the legal technicalities work of copying code form an MIT-licensed project into an Apache licensed one

jkillian · 2016-03-31T20:54:44Z

Also, I like how simple & clean this code is ending up for such a powerful feature! This will be a great PR once it gets merged in

unional · 2016-03-31T21:00:31Z

Ok

What you think on the exception message for resolve?

unional · 2016-03-31T21:04:16Z

Also, for the typings for path-is-abs, I have added it to typings. But adding to DT might take a while. What's your approach? Custom?

jkillian · 2016-03-31T21:15:16Z

What you think on the exception message for resolve?

Maybe something like this:

Error: Invalid "extends" configuration value - could not require "some-invalid-path". Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) for the approximate method TSLint uses to find the referenced configuration file.

Also, for the typings for path-is-abs, I have added it to typings. But adding to DT might take a while. What's your approach? Custom?

Seems like a good solution to me, just drop it in typings/custom.

unional · 2016-03-31T21:32:36Z

Ok. I drop the file under /custom-typings/ instead of /typings/custom to differentiate it from the typings installed by tsd.

Should be good to go V2!

adidahiya · 2016-03-31T21:44:33Z

src/configuration.ts

@@ -78,7 +79,7 @@ export function findConfiguration(configFile: string, inputFilePath: string): IC
 /**
 * Searches for a TSLint configuration and returns the path to it.
 * Could return undefined if not configuration is found.
- * @param suppliedConfigFilePath A path to an known config file supplied by a user. Pass null here if
+ * @param suppliedConfigFilePath A path to an know``n config file supplied by a user. Pass null here if


Ouch

UPDATE: DONE!

jkillian · 2016-04-01T01:38:30Z

Sweet! Comments I left are just minor style things, I'll make the tweaks in a follow-up PR if you don't have a chance to do it yourself. Also, can you squash this into one commit now?

unional · 2016-04-01T01:53:40Z

Sure

unional · 2016-04-01T04:13:31Z

Regarding the declare namespace ... trick, it is neat and I haven't thought about that before.

I don't know if you aware of microsoft/TypeScript#7398. There may be problem in the future.

Just want to bring it up to your attention.

jkillian · 2016-04-01T14:27:39Z

🎈 🎉

jkillian · 2016-04-02T01:03:00Z

I wrote a little post about this: http://palantir.github.io/tslint/2016/03/31/sharable-configurations-rules.html

Does it all seem correct to you @unional?

unional · 2016-04-02T01:15:34Z

Seems like you go all the way to the complex scenario. 😄
Haven't try the tslint-crazy-config.js route myself. But given of what it is, it should work.

The simplest case would be:

// package.json
{
  "main": "tslint.json"  // or any json file
}

// tslint.json
{
  "rulesDirectory": "./rules",
  "rules": {
     // default values
  }
}

Then if you want to extend from someone else, just change your tslint.json to:

// tslint.json
{
  "extends": "some-other-package",
  "rulesDirectory": "./rules",
  "rules": {
     // my rules default
     // overriding `some-other-package` defaults
  }
}

unional · 2016-04-02T01:18:41Z

In your example you are suggesting the package authors to share multiple configs. It is powerful, but it is also more advanced. 🌹

unional · 2016-04-02T01:28:06Z

So you may organize the post into three sections:

how do I use it as the end user
how do I use it as a package author extending other packages (i.e. adding my own touch)
how do I use it as a package author to expose multiple configuration for my user to use.

My two cents.

unional changed the title ~~Add basic support~~ Add basic extends support Mar 5, 2016

adidahiya assigned jkillian Mar 5, 2016

adidahiya added the PR: waiting for reviewer label Mar 5, 2016

jkillian reviewed Mar 7, 2016
View reviewed changes

unional force-pushed the support-extends branch from 43a52dc to 88abb78 Compare March 8, 2016 01:02

unional reviewed Mar 8, 2016
View reviewed changes

unional mentioned this pull request Mar 9, 2016

Soon: May use extends buzinas/tslint-eslint-rules#53

Closed

unional changed the title ~~Add basic extends support~~ Add extends support Mar 31, 2016

jkillian added the PR: waiting for author label Mar 31, 2016

adidahiya reviewed Mar 31, 2016
View reviewed changes

unional force-pushed the support-extends branch from 9c15827 to 7168e6a Compare April 1, 2016 02:50

Implement extends feature

747e7a5

unional force-pushed the support-extends branch from 7168e6a to 747e7a5 Compare April 1, 2016 03:23

jkillian removed the PR: waiting for author label Apr 1, 2016

jkillian merged commit 5b0eb9a into palantir:master Apr 1, 2016

unional mentioned this pull request Apr 15, 2016

Add declare namespace typed-typings/npm-any-promise#1

Closed

Add extends support #1016

Add extends support #1016

Conversation

unional commented Mar 5, 2016

unional commented Mar 5, 2016

unional commented Mar 6, 2016

unional commented Mar 6, 2016

unional commented Mar 6, 2016

jkillian commented Mar 7, 2016

unional commented Mar 7, 2016

jkillian Mar 7, 2016

Choose a reason for hiding this comment

unional Mar 7, 2016

Choose a reason for hiding this comment

unional commented Mar 7, 2016

jkillian commented Mar 7, 2016

unional commented Mar 7, 2016

unional commented Mar 7, 2016

jkillian commented Mar 7, 2016

unional commented Mar 7, 2016

unional commented Mar 8, 2016

unional commented Mar 8, 2016

jkillian commented Mar 8, 2016

unional commented Mar 8, 2016

unional commented Mar 8, 2016

unional Mar 8, 2016

Choose a reason for hiding this comment

jkillian Mar 9, 2016

Choose a reason for hiding this comment

unional Mar 9, 2016

Choose a reason for hiding this comment

unional commented Mar 9, 2016

unional commented Mar 31, 2016

unional commented Mar 31, 2016

jkillian commented Mar 31, 2016

jkillian commented Mar 31, 2016

unional commented Mar 31, 2016

unional commented Mar 31, 2016

jkillian commented Mar 31, 2016

unional commented Mar 31, 2016

adidahiya Mar 31, 2016

Choose a reason for hiding this comment

unional Mar 31, 2016

Choose a reason for hiding this comment

jkillian commented Apr 1, 2016

unional commented Apr 1, 2016

unional commented Apr 1, 2016

jkillian commented Apr 1, 2016

jkillian commented Apr 2, 2016

unional commented Apr 2, 2016

unional commented Apr 2, 2016

unional commented Apr 2, 2016