Add OUTPUT-FILTER option

[purcell]

Closes #25, #26: this is similar but avoids leaving dangling temp files.

An eslint --fix formatter written with this facility looks like this:

  (reformatter-define eslint-fix
    :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-filter (lambda ()
                     (let-alist (elt (json-read-array) 0)
                       (when .output
                         (delete-region (point-min) (point-max))
                         (insert .output)
                         t))))

However, this fails when there are no fixes applicable to the current buffer, because no replacement output is included. We would need a structured way for the output-filter function to indicate that it has determined there are no changes to be made. eslint --fix returns a zero exit code in either case.

[wyuenho]

Looks good! Regarding #26 (comment), CLI command exit statuses are program specific. In the case of eslint, swaping the exit-code-success-p to integerp like so solves that problem.

[purcell]

In the case of eslint, swaping the exit-code-success-p to integerp like so solves that problem.

I'm not convinced that's the case. I just updated the PR description to reflect my findings in this area.

[wyuenho]

Ah right, I looked over the filter implementation here. I guess in this implementation you can change the signature of the filter function to accept the input buffer and the output temp buffer, and replace the input buffer with whatever is returned from the filter function. When there's no output, the function can just return the input as is.

Alternatively, an even easier solution will be if the filter function returns nil, don't replace the input buffer content. In this case something like the following will work:

:output-filter (lambda ()
                         (let ((data (ignore-error 'json-end-of-file (json-read))))
                           (and data (arrayp data) (alist-get 'output (aref data 0)))))

[wyuenho]

Ok nevermind, I see where you are going with this. I just remembered we'll want to preserve mark and point with insert-file-contents.

In that case I maintain something similar to #26 is still the simpler solution. If there aren't any output the processor just needs to return the file as is.

:output-processor (lambda (input-file output-file)
                        (let* ((data (ignore-error 'json-end-of-file (json-read-file output-file))
                               (output (and data (arrayp data) (alist-get 'output (aref data 0)))))
                          (if output (make-temp-file "eslint-format" nil nil output) input-file)))

If the user cares about cleaning up the temp file, they can append a hook in before-save-hook.

[purcell]

an even easier solution will be if the filter function returns nil, don't replace the input buffer content.

Yes, potentially.

In that case I maintain something similar to #26 is still the simpler solution. If there aren't any output the processor just needs to return the file as is.
:output-processor (lambda (input-file output-file)

This exposes more info to the function than is necessary. The return value mechanism is more elegant.

If the user cares about cleaning up the temp file, they can append a hook in before-save-hook.

That's a degree of unnecessary complication that can and should be avoided by design.

[wyuenho]

Take a look at the latest changes here

With regard to the temp file, hundred of programs leave temp files in the file system without cleaning them up when exiting. We aren't writing a server here where you'll run the risk of running out of inodes. Just leave OS problems to the OS.

[wyuenho]

This commit will supply a callback to the processor. That callback will automatically create a temp file, and will be deleted after insertion. All your concerns are addressed now. Let me know if you want me to submit a new PR.

You can now write your processor like so:

    :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)))

[purcell]

The big issue with all of this is error handling. When eslint --fix-dry-run fails, the exit code is zero, but error details are included in the JSON output, but silently thrown away in both our solutions, and that gets fiddly to handle and report robustly.

There's a pattern here that we're missing by hacking in an extra function, since each step (running the command, processing its output) is a consecutive reformatting step that receives input and can either skip, produce replacement text, or produce error output. I have some ideas about how to handle that more consistently and simply, and will likely pursue that in place of this approach.

[wyuenho]

That sounds like a state machine. It's an admirable goal, but the implementation in this PR is entirely fine, the error handling problem isn't new and it's been around since there's an :exit-code-success-p, perhaps we can get some incremental improvements first?

[wyuenho]

Happy new year! Can we merge this please? I'd rather have this than nothing at all.