Enabling the fix flag for stdio

[fatfisz]

The problem

I'm using SublimeLinter-contrib-eslint and I noticed that it operates on stdio. This is probably because the linter also works on non-yet-saved-files. I wanted to enable autofixing of the code, but it was not built-in and when manually passing the --fix flag I saw that ESLint just exits with an error message. I decided that enabling --fix and --stdin together was the right first step to autofixing with the linter plugin.

The solution

Currently there are three proposed solutions:

1. The one I've prepared a PR for (Add the fix-dry-run flag #9073), that utilizes a non-standard file descriptor (number 3, just after 0, 1, 2 for stdin, stdout, stderr respectively) for providing the fixed output.
2. The one that comes from @not-an-aardvark, that suggests just removing the existing restriction and allowing the use of a custom formatter to retrieve the output.
3. Add a new flag like --fix-dry-run that can be used with --stdin, but doesn't output fixes to the filesystem.

I'd be happy to go with either of them, and frankly, the second one feels like a more convenient option to use.

(edited by @not-an-aardvark to add the third proposed solution)

[not-an-aardvark]

Thanks for creating an issue. I'm 👍 for just removing the restriction on --fix with --stdin -- it seems like it would solve the problem without needing to introduce any special behavior.

There is a slight potential for confusion if someone uses --fix --stdin with a formatter that doesn't display output (e.g. the default stylish formatter), because it won't be clear that any output was produced. But that's fine in my opinion -- we can just document the behavior and tell people to use the json formatter (or another formatter that includes output). --stdin is probably used more by integrations than by people typing on the command line anyway, so having to add a flag for the formatter isn't too much of an inconvenience.

[platinumazure]

I'm not sure about this, because I don't know how we can easily display both the reported messages (remaining lint errors/warnings) and the fixed code, in an unambiguous way.

I would be curious about having --stdin --fix output only fixed code, with no reporting of remaining issues. Then an editor integration could call the CLI with --stdin --fix, apply the fixed code to the buffer, and then call with --stdin to get remaining lint errors.

[not-an-aardvark]

I'm not sure about this, because I don't know how we can easily display both the reported messages (remaining lint errors/warnings) and the fixed code, in an unambiguous way.

@platinumazure To clarify, proposal 2 only removes the restriction agains --fix --stdin, and makes no other changes. Formatters that only display problems, such as stylish, would continue to do so. On the other hand, formatters that can display both problems and output, such as json, would be able to unambiguously do that with the --stdin flag.

The output property is already passed to formatters when linting files with --fix, but stylish ignores it.

I would be curious about having --stdin --fix output only fixed code, with no reporting of remaining issues. Then an editor integration could call the CLI with --stdin --fix, apply the fixed code to the buffer, and then call with --stdin to get remaining lint errors.

I'm not sure I like this idea because it seems like a surprising behavior switch. When no flags are used, or when --fix is used, or when --stdin is used, eslint outputs problems. It seems unexpected that when --fix and --stdin are used together, eslint suddenly outputs text.

(edit: added a response to the second paragraph)

[platinumazure]

So to make sure I understand the proposal, would CLIEngine.outputFixes (or an equivalent for stdin) be called at all? I'm still not really seeing the utility of doing this since only certain formatters would even provide useful output for this use case. My preference would be for us to come up with a more well thought-out design that doesn't depend on a third CLI option for its effectiveness.

…

On Aug 5, 2017 10:49 PM, "Teddy Katz" ***@***.***> wrote: I'm not sure about this, because I don't know how we can easily display both the reported messages (remaining lint errors/warnings) and the fixed code, in an unambiguous way. @platinumazure <https://github.com/platinumazure> To clarify, proposal 2 *only* removes the restriction agains --fix --stdin, and makes no other changes. Formatters that only display problems, such as stylish, would continue to do so. On the other hand, formatters that can display both problems and output, such as json, would be able to unambiguously do that with the --stdin flag. The output property is already passed to formatters when linting files with --fix, but stylish ignores it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9076 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWehKuTQqq0UPvojASr0Frhv7GcKQrks5sVTevgaJpZM4OulNy> .

[not-an-aardvark]

So to make sure I understand the proposal, would CLIEngine.outputFixes (or
an equivalent for stdin) be called at all?

Right, it would not get called when the text comes from stdin. Admittedly, this would be a special case, which does seem undesirable.

I'm still not really seeing the utility of doing this since only certain
formatters would even provide useful output for this use case.

The utility is for integrations that want to autofix text without writing it to the filesystem. I agree that only some formatters would be useful here -- my assumption is that integrations would use only the useful formatters for this use case. (Integrations that use the command-line interface typically use those formatters anyway, rather than the human-readable formatters.)

[platinumazure]

Thanks @not-an-aardvark.

I guess I'm worried about CLI end-user confusion if they try to use --stdin --fix with a formatter like stylish, and then not being sure what is and is not happening. In terms of providing clear APIs/options to users, this feels like a step in the wrong direction to me. Unfortunately, I don't have any better proposal at this time, so I don't want to stand in the way, but I want to make my (not minor) concerns clear on the record.

[not-an-aardvark]

How about this proposal?

• Add a new flag: --fix-dry-run. This flag is mutually exclusive with --fix. When this flag is active, ESLint runs in "fix mode" and outputs only unfixed problems, but doesn't write anything to the filesystem.
• --fix is basically the same as --fix-dry-run, except that it also writes the output to the filesystem.
• --stdin and --fix are still mutually exclusive, but --stdin and --fix-dry-run can be used together.

I like this proposal because it doesn't introduce any special cases, and seems like it obeys the principle of least astonishment. --fix always outputs text to the filesystem (or throws a fatal error where it doesn't make sense to do so), and --fix-dry-run never outputs text to the filesystem. In either case, you can also get information about fixes if you use a formatter which includes that information.

[fatfisz]

I'm not sure about --fix-dry-run; ESLint is already not the fastest tool, and doing the same thing two times only to present two different parts of the output seems like a small usability gain for a big perf hit. It's especially bad in the context of editing, when I'd like to have almost immediate feedback.

I think the people most interested in this will be the plugin or the tool authors, so it should be sufficient to document the feature. Additionally, we could make a getter for report.results.output that would save info about it being accessed - if the output was not accessed we could include a message about using a custom formatter.

[not-an-aardvark]

doing the same thing two times only to present two different parts of the output seems like a small usability gain for a big perf hit.

I think you're misunderstanding the proposal somewhere; --fix-dry-run would not require ESLint to be run twice. The only difference between --fix and --fix-dry-run is that when using --fix, ESLint modifies the filesystem in addition to printing output. When using --fix-dry-run, ESLint only prints output, and integrations can use that output to modify the filesystem (or e.g. a text buffer) themselves.

[fatfisz]

When using --fix-dry-run, ESLint only prints output

I'm addressing this - not everything is fixable, so if i wanted to get both the warnings/errors and the fixed output at the same time, I'd need to run ESLint twice. Or not?

[fatfisz]

Could I use --stdin and --fix-dry-run together with a custom formatter to get everything in one go?

[not-an-aardvark]

I'm addressing this - not everything is fixable, so if i wanted to get both the warnings/errors and the fixed output at the same time, I'd need to run ESLint twice. Or not?

No -- to clarify, by "output" I mean the object that is usually passed to a formatter, which looks something like this:

Result list

[
  {
    "filePath": "/path/to/foo.js",
    "messages": [
      {
        "ruleId": "no-undef",
        "severity": 2,
        "message": "'foo' is not defined.",
        "line": 1,
        "column": 1,
        "nodeType": "Identifier",
        "source": "foo;",
        "endLine": 1,
        "endColumn": 4
      }
    ],
    "errorCount": 1,
    "warningCount": 0,
    "fixableErrorCount": 0,
    "fixableWarningCount": 0,
    "output": "foo;\n"
  }
]

Note that this includes the autofix output in the "output" property. (This output came from a run where the no-undef and semi rules were enabled, and the semi problem was autofixed, so only the no-undef problem was included in the problem list.)

Could I use --stdin and --fix-dry-run together with a custom formatter to get everything in one go?

Yes, that's the idea. You wouldn't even need a custom formatter -- you could use the built-in json formatter, which just takes objects like the one above and outputs them as JSON.

[fatfisz]

I'll work on this during the weekend then. Unfortunately my time during the week is quite limited.

[not-an-aardvark]

Thanks! I should note that this proposal is still marked as "evaluating", so there is still a possibility that it will change again after more discussion (which would result in you having to reimplement it again.) If you want to be sure that you won't accidentally waste your time on an implementation that doesn't get used, you could also choose to wait until the proposal is marked as "accepted".

[fatfisz]

I'll wait then! Please ping me when it gets changed, because there are no notifications for the labels.

[not-an-aardvark]

@platinumazure Any thoughts on the updated proposal in #9076 (comment)? If that proposal addresses your objections, then I'd like to add this to the TSC agenda for approval. Otherwise, we can discuss more to hopefully find the best solution.

[platinumazure]

I'm okay with that proposal, yes. Thanks for following up.

[not-an-aardvark]

TSC Summary: This issue proposes adding a --fix-dry-run flag to the CLI, which identical to the --fix flag except that it doesn't write the output to the filesystem. This would allow command-line integrations to autofix text when using the --stdin flag, by using --fix-dry-run in combination with a formatter like json that includes the output property.

TSC Question: Should we accept this?

[not-an-aardvark]

This was approved in today's TSC meeting.

cc @fatfisz

[fatfisz]

Snap, I forgot about this - will do it tomorrow!