fix #25 hook for post-processing formatter output

[wyuenho]

This enables eslint to work with reformatter using the following configuration:

(reformatter-define eslint-format
    :program "eslint"
    :args `("--fix-dry-run"
            "--format"
            "json"
            "--stdin"
            "--stdin-filename"
            ,buffer-file-name
            "--ext"
            ".json,.js,.jsx,.mjs,.mjsx,.cjs,.cjsx,.ts,.tsx")
    :output-processor (lambda (output-file result-callback)
                        (let* ((data (ignore-error 'json-end-of-file (json-read-file output-file)))
                               (output (and data (arrayp data) (alist-get 'output (aref data 0)))))
                          (funcall result-callback output)))
    :exit-code-success-p integerp)

[wyuenho]

@purcell how does this look?

[purcell]

Thanks. I'm going to have a deeper think about how best to support extending reformatter in this sort of way. I'm wary of ending up with lots of special keyword arguments, so I want to plan this a little.

[wyuenho]

How's the deep think going?

[purcell]

I started doing a bit of hacking a week or two ago and I think I'm getting a clearer picture of how things should work. Hoping to have some free time this week.

[purcell]

Now that I've landed #27, this works:

  (defcustom eslint-fix-program "eslint"
    "Program name or path for eslint."
    :type 'string)
  (defcustom eslint-fix-arguments nil
    "Extra arguments for eslint --fix, e.g.'(\"--no-eslintrc\")."
    :type '(list string))
  (reformatter-define eslint-fix
    :program eslint-fix-program
    :stdin nil
    :stdout nil
    :input-file (reformatter-temp-file-in-current-directory "js")
    :args (append eslint-fix-arguments (list "--fix" input-file)))

[wyuenho]

So... in an effort to reduce options, you've managed to introduce more options than this PR lol

What's the advantage of introducing stdin stdout and input-file ?

[purcell]

So... in an effort to reduce options, you've managed to introduce more options than this PR lol

Erm, this PR is for a different purpose, so I don't understand your "lol" at all. But it just so happens that with my changes in master, you no longer need to post-process JSON output from eslint, hence my example above (which I'm actually using locally for a project).

What's the advantage of introducing stdin stdout and input-file ?

I think the docs for those options should explain that.

[purcell]

Suffice to say I spent a number of hours thinking about the modes of operation and configuration I would want reformatter.el to support with respect to file/stream input/output, and this was what I came up with. Let me know if you come across any issues.

[wyuenho]

The docstring just tells me what these options are but not the rationale. Granted eslint is the odd one out but your PR requires 4 different things to be set right for eslint to work. Are there code formatters where its only mode of operation is to require a file path and puts out the result in stdout or vice versa? This approach also requires making a temp copy of the file instead of just sending the buffer content, because, in-place. Also, presumably the user won't even notice the file was formatted if auto-revert-mode wasn't turned on.

AFAIK every modern code formatter I know of supports stdin/stdout mode, even isort, so sticking your gun to only supporting stdin/stdout mode can go a really long way. The only variable is some formatters will adorn the output with extra metadata, that's why you need to post process the output. I don't understand the necessity of #27.

[purcell]

Are there code formatters where its only mode of operation is to require a file path and puts out the result in stdout or vice versa?

Hmm, thanks, you could be right. I just took a look at format-all.el and couldn't see any cases where temp files were required. I definitely want support for post-processing output too, see #24 (comment) -- maybe I'll shrink the interface again once that is done.

[wyuenho]

There's a reason I kept asking you what you were thinking man 😉

[purcell]

There's a reason I kept asking you what you were thinking man 😉

Yeah, if you had said: "Are there code formatters where its only mode of operation is to require a file path and puts out the result in stdout or vice versa?" initially instead of "in an effort to reduce options, you've managed to introduce more options than this PR lol" this whole conversation would have gone a lot smoother.

[wyuenho]

I asked What's the advantage of introducing stdin stdout and input-file ? It took for 4 comments over a span of 5 months to get to where we are, and I still don't know what the rationale of #27 was. I'm not here to offend you, I just want to get a discussion going and solve problems. Programming in silos actually affects people you know.

[purcell]

Let's get concrete. I think there are 2 issues with this PR:

• Minor: the additional temp file created by the output-processor function never gets deleted.
• Blocker: when there's nothing for the reformatter to do, no "output" is produced, so the original buffer is either replaced with no content or there's an error, more likely the second case. More careful design would likely be necessary in order to allow defined reformatters to signal when the operation is known to be a no-op, either following inspection of the output text, or based on the exit code.

I've reimplemented this PR with tests in #28, in a different way such that it avoids dangling temp files, but the no-op issue still needs to be addressed. In the meantime, for eslint --fix specifically, the solution I posted with :stdin etc. works fine, regardless of the long-term desirability of such options. I see that format-all doesn't handle eslint --fix either. I'm not convinced of the usefulness of merging #28 in isolation, though, given the "no-op" issue.

[wyuenho]

I've addressed the blocker and updated the snippet in the description. If anyone is interested, this PR should work with eslint via stdin/stdout without any major issues. If you really care about deleting the temp file, you can save the temp file name in some global variable and clean up it up by appending a function to before-save-hook. I'm not going to bother because any reasonable OS will clean them up periodically.

There is now a callback supplied that will automatically create a temp file for the output, and it will be clean up for reformatter afterwards.