Implement --stdin and --stdin-filename CLI options #3816

w0rp · 2018-04-06T10:18:39Z

PR checklist

Addresses an existing issue: Accept input from stdin? #1590
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Hello! I am the guy who created the ALE plugin for Vim, for checking for problems while you type in Vim, and I am an active JavaScript and TypeScript developer. I use ESLint in my day job, and I would like to use TSLint more. One of the key features that ESLint has, which works really well with ALE, is that you can both pass in a Vim buffer via stdin and also tell it where the file for the buffer is in your file system. This means you can check for problems before you've saved a file to disk, but still benefit from plugins which need to know where the file is for resolving modules and so on. Currently, ALE can't support all of the features of TSLint, while also checking for problems while you type, because TSLint doesn't yet support a feature like this.

I'm interested in using such a feature myself, so I thought I'd start trying to implement it myself. A new --stdin option ought to read a file to check from stdin, and --stdin-filename should tell TSLint where the file you're checking will be saved to disk.

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

I'm not familiar with the TSLint codebase at all. I just quickly scanned through the codebase and modified what seemed logical to modify to me, to get anything working at all. I haven't written any tests yet, and I'm sure what I've written so far will probably cause some bugs. I'm not familiar with the coding standards, so let me know what to change there. This work certainly isn't finished yet.

One thing I know that I couldn't get working yet is that I couldn't figure out an easy way to support --stdin without the --stdin-filename option, as some rules seem to depend on there being some file to actually look at in the filesystem, and I imagine this will extend to some third party rules as well. It might be that some rules will have to be modified to not throw exceptions if the file doesn't exist yet, they will probably cause problems for new files you haven't saved to disk yet, even with the --stdin-filename option. Maybe for supporting --stdin alone, rules will have to be modified to tolerate blank filenames, or a special filename like "-". Let me know your thoughts.

CHANGELOG.md entry:

suggested tags: [enhancement], [api]

palantirtech · 2018-04-06T10:18:40Z

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

w0rp · 2018-04-06T10:24:51Z

Also, fixing code via --stdin should be considered, but I'd probably focus on getting checking for errors implemented first, and implement fixing problems automatically second. It's worth keeping it in mind while working on this.

w0rp · 2018-04-23T19:42:41Z

I'd appreciate some feedback on this if anyone wants to take a look. I imagine quite a bit more work will need to be done before this is good enough.

jaalzateolaya

Changes are required to pass tests

jaalzateolaya · 2018-04-24T15:13:21Z

src/runner.ts

@@ -157,7 +167,9 @@ async function runWorker(options: Options, logger: Logger): Promise<Status> {
 }

 async function runLinter(options: Options, logger: Logger): Promise<LintResult> {
-    const { files, program } = resolveFilesAndProgram(options, logger);
+    const program = resolveProgram(options);
+    const files = await resolveFiles(options, program, logger);


You aren't returning a Promise here.

jaalzateolaya · 2018-04-24T15:17:16Z

src/runner.ts

-function resolveFilesAndProgram(
-    { files, project, exclude, outputAbsolutePaths }: Options,
+/** Read the stdin stream into a string with all of the contents. */
+function readStdin(): Promise<string> {


The signature should be:

async function readStdin(): Promise<string>

jaalzateolaya · 2018-04-24T15:24:08Z

src/tslint-cli.ts

    console.error("No files specified. Use --project to lint a project folder.");
    process.exit(1);
 }

+if (argv.stdin && !argv.stdinFilename) {


strict-boolean-expressions forces this expression to be:

if (arg.stdin && argv.stdinFilename !== undefined) { // code... }

w0rp · 2018-04-24T16:44:54Z

Thanks for the feedback. I'd appreciate some in-depth feedback about how to handle just --stdin without --stdin-filename, what might go wrong, etc.

jaalzateolaya · 2018-04-24T16:49:56Z

Perhaps you should also change the subject of the commit, maybe something like:

"Implement --stdin and --stdin-filename CLI options"

suchanlee · 2018-04-26T18:52:20Z

Hey @w0rp thank you for your contribution! Just wanted to let you know that I'll be looking into this PR probably sometimes next week (swamped with work this week). I think that this is really interesting and can be a very powerful feature.

w0rp · 2018-04-26T19:18:10Z

Okay, sounds good. Thanks for letting me know. 👍

giladgray · 2018-05-01T21:38:49Z

src/runner.ts

+
+        process.stdin.on("error", (error: Error) => {
+          reject(error);
+        });


indentation is wrong throughout this file. some 4 spaces, some 2 spaces.

I'll fix that when I get the time. I haven't managed to get tslint running on tslint code itself yet.

giladgray · 2018-05-01T21:39:25Z

src/runner.ts

+    stdin?: boolean;
+
+    /**
+     * Tell TSLint where the file for some stdin data is.


this language is awkward. what is "some stdin data?" please clarify this before merging.

Throw whatever string you want at me, and I'll write that.

How about:

Tell TSLint where the actual file being linted through stdin is, if any.

Okay, done. 👍

w0rp · 2018-05-26T10:06:53Z

@suchanlee Will you have some time to look at this soon? I'm not familiar with the TSLint codebase at all, but I'm willing to do the work to get this feature done, with some guidance. I get reports of issues with TSLint integration with ALE pretty much every week.

jaalzateolaya · 2018-05-07T01:50:48Z

src/runner.ts

+    stdin?: boolean;
+
+    /**
+     * Tell TSLint where the file for some stdin data is.


How about:

Tell TSLint where the actual file being linted through stdin is, if any.

suchanlee · 2018-06-26T21:13:51Z

src/runner.ts

-        if (program !== undefined) {
+
+        if (resolvedFile.isStdin) {
+            contents = await readStdin();


is --std-filename being ignored when actually fetching content with --std?

Essentially yes. The filename is a hint for where the file is, and the data comes from stdin.

suchanlee · 2018-06-26T21:33:13Z

src/tslint-cli.ts

+  || argv.project !== undefined
+  || commander.args.length > 0
+  || argv.stdin
+)) {
    console.error("No files specified. Use --project to lint a project folder.");


might be a good idea to mention --stdin here

suchanlee · 2018-06-26T21:34:20Z

src/runner.ts

+            // Use a filename for shadowing stdin, if the option is set.
+            return [{filename: path.resolve(stdinFilename), isStdin: true}];
+        } else {
+            return [{filename: "", isStdin: true}];


maybe for now you should fail hard here with a good error message, until --stdin w/o --stdinFilename is supported

suchanlee · 2018-06-26T21:35:43Z

src/tslint-cli.ts

+        type: "string",
+        describe: "stdin input",
+        description: dedent`
+            Read and check a file via stdin.`,


would mention that stdin-filename is currently required

suchanlee · 2018-06-26T21:58:44Z

the way it currently stands, it looks like if the file(s) to lint is not specified, the program would fail at Linter.lint when getSourceFile is called: https://github.com/palantir/tslint/blob/master/src/linter.ts#L250

One way to hack around it is to create a ts.SourceFile object from the lint method when using --stdin using ts.createSourceFile (or the utils.getSourceFile wrapper), since we have the source available in the Linter.lint method and can generate something for the file name. And as long as we have a valid ts.SourceFile object, I dont think any of the rules will break. Some of them may malfunction though, esp noImplicitDependenciesRule, noReferenceImportRule, and noUnusedVariableRule (deprecated), but we might be able to do something about those as well, with the way we construct the ts.SourceFile.

I am also open to incrementally developing this, with the version that requires --stdin-filename first and then working through to support one that doesn't require the argument.

w0rp · 2018-06-27T10:08:54Z

Thank you for the feedback. 👍 I'll have a read through all of your comments when I get the time. I was trying to figure out how to support --stdin without --stdin-filename. I agree that it might be a good idea to just require --stdin-filename at first, and add support for --stdin without --stdin-filename later.

w0rp · 2018-07-07T10:49:41Z

I think I'll go with the option to require --stdin-filename at first, and someone can work on supporting --stdin without --stdin-filename later.

w0rp · 2018-07-07T11:05:26Z

I updated the code now to require --stdin-filename for now. Let me know what else I can do. Perhaps there's some way I can cover some of this with tests. I haven't looked at the tests yet.

blueyed · 2018-09-09T18:51:09Z

@w0rp
There are some test failures - maybe you have a test there already that just needs to be adjusted?

tommck · 2018-10-27T15:57:57Z

I'm waiting anxiously for this to be finished so I can integrate the tslint-cli into the Visual Studio Prettier extension.

Any status? Anything I can help with?

w0rp · 2018-10-27T16:26:13Z

You can take what I've written so far and add to it. I don't need this any more. I just run tslint as a tsserver plugin instead.

JoshuaKGoldberg

Tests will need to pass before this is merged.

JoshuaKGoldberg · 2018-11-07T00:11:20Z

src/runner.ts

-function resolveFilesAndProgram(
-    { files, project, exclude, outputAbsolutePaths }: Options,
+/** Read the stdin stream into a string with all of the contents. */
+async function readStdin(): Promise<string> {


runner.ts is getting pretty huge. Please separate this standalone utility to a different file (`src/utils.ts, maybe?).

tommck · 2018-11-09T14:34:20Z

It's taking me a bit to get used to the code base, but I'll try to get to this. It won't be too soon though.

If anyone else wants to do it, it would be great. Just let me know.

tommck · 2018-11-15T17:48:23Z

So, just to clarify what's needed now:

Need to extract the stdin reading into an external file (possibly src/utils.js)
Need to get unit tests passing.

Anything else?

JoshuaKGoldberg · 2018-11-17T00:01:08Z

@tommck looks like it, yes. Also a few merge conflicts now 😉 but those are probably just from a recent Prettier reformatting.

adidahiya · 2019-01-09T01:14:18Z

given what @w0rp said here:

I don't need this any more. I just run tslint as a tsserver plugin instead.

is this feature still required? what are the downsides to running tslint as a tsserver plugin?

blueyed · 2019-01-09T08:15:05Z

is this feature still required? what are the downsides to running tslint as a tsserver plugin?

I would still use it in Neomake I guess, which does not has support for tsserver plugins.
It is still useful for running tslint on its own.

w0rp · 2019-01-09T09:37:09Z

I think it's worth implementing for various purposes, including piping text into TSLint. Someone else can pick this up. I personally recommend running TSLint as a tsserver plugin, as that seems to work very well.

JoshuaKGoldberg · 2019-06-15T23:34:33Z

Given #4534 and this PR's failing tests, merge conflicts, and lack of participation in the last few months, I'm going to go ahead and close this PR for housekeeping. Thanks so much everyone for participating in the PR! Although the changes haven't been merged in, there was still some great work that went into this. 😢

Anybody: if piping text via --stdin / --stdin-filename is still relevant to you, please file a separate PR that addresses the comments above.

cpitclaudel mentioned this pull request Apr 14, 2018

Is it changeable flycheck temporary files path? flycheck/flycheck#1446

Closed

jaalzateolaya suggested changes Apr 24, 2018

View reviewed changes

w0rp changed the title ~~Experiment with --stdin and --stdin-filename support for tslint~~ Implement --stdin and --stdin-filename CLI options Apr 24, 2018

w0rp force-pushed the stdin-support branch from b8ba5aa to e8cf90b Compare April 24, 2018 18:43

giladgray reviewed May 1, 2018

View reviewed changes

w0rp force-pushed the stdin-support branch from e8cf90b to 599b42f Compare May 3, 2018 20:36

w0rp mentioned this pull request May 15, 2018

tslint command broken for tslint > 5.10 dense-analysis/ale#1579

Closed

w0rp mentioned this pull request May 24, 2018

feauture request: support tslint rules that require type-check dense-analysis/ale#1286

Closed

aminer-looker mentioned this pull request May 26, 2018

Tell TSLint what the filename is for temporary files (Changes needed to be made to TSLint itself) dense-analysis/ale#925

Closed

jaalzateolaya approved these changes May 27, 2018

View reviewed changes

w0rp force-pushed the stdin-support branch from 599b42f to af374fd Compare May 27, 2018 06:57

alecmev mentioned this pull request Jun 7, 2018

Can we make it search "up" for a config? SublimeLinter/SublimeLinter-tslint#32

Closed

suchanlee reviewed Jun 26, 2018

View reviewed changes

w0rp added 2 commits July 7, 2018 12:04

Experiment with --stdin and --stdin-filename support for tslint

8f93bfb

Require --stdin-filename for --stdin for now

c033b17

w0rp force-pushed the stdin-support branch from be3b9f8 to c033b17 Compare July 7, 2018 11:04

euclio mentioned this pull request Jul 13, 2018

Allow disabling project file check when using -p flag #3677

Closed

euclio mentioned this pull request Jul 20, 2018

[blocked upstream] parse tslint JSON output neomake/neomake#1829

Closed

JoshuaKGoldberg added PR: waiting for author PR: merge target branch labels Nov 7, 2018

JoshuaKGoldberg suggested changes Nov 7, 2018

View reviewed changes

adidahiya added Status: In Discussion and removed PR: waiting for author labels Jan 9, 2019

JoshuaKGoldberg closed this Jun 15, 2019

JoshuaKGoldberg added Resolution: Aged Away and removed Status: In Discussion labels Jun 15, 2019

Implement --stdin and --stdin-filename CLI options #3816

Implement --stdin and --stdin-filename CLI options #3816

Conversation

w0rp commented Apr 6, 2018

PR checklist

Overview of change:

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

CHANGELOG.md entry:

palantirtech commented Apr 6, 2018

w0rp commented Apr 6, 2018

w0rp commented Apr 23, 2018

jaalzateolaya 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

w0rp commented Apr 24, 2018

jaalzateolaya commented Apr 24, 2018

suchanlee commented Apr 26, 2018

w0rp commented Apr 26, 2018

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

w0rp commented May 26, 2018

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

suchanlee commented Jun 26, 2018

w0rp commented Jun 27, 2018

w0rp commented Jul 7, 2018

w0rp commented Jul 7, 2018

blueyed commented Sep 9, 2018

tommck commented Oct 27, 2018

w0rp commented Oct 27, 2018

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommck commented Nov 9, 2018

tommck commented Nov 15, 2018

JoshuaKGoldberg commented Nov 17, 2018

adidahiya commented Jan 9, 2019

blueyed commented Jan 9, 2019

w0rp commented Jan 9, 2019

JoshuaKGoldberg commented Jun 15, 2019 • edited Loading

JoshuaKGoldberg commented Jun 15, 2019 •

edited

Loading