How to set :exit-code-success-p conditioned on the `sqlformat-command` variable?

[tah-alv]

The latest release of sqruff added support for reading from stdin and I was starting a PR to add sqruff as an option for the sqlformat-command. However, the meaning of the exit code for sqruff is a bit different than the other formatting engines supported by sqlformat. AFAICT in sqruff an exit code of 0 means that no formatting changes were applied to the input, an exit code of 1 means that formatting changes were applied but that the input text was correctly parsed and processed by sqruff, while an exit code of 2 means that there was some issue with the input text. So this means that the exit-code-success-p arg should be set to something like (lambda (exit-code) (< exit-code 2)) for sqruff and zerop for the other formatting engines.

I tried to do this here but the (cl-assert (functionp exit-code-success-p)) assertion fails in the reformatter-define macro in this case. I don't know lisp very well but I'm guessing this has to do with the argument not being evaluated before being passed to the macro. Is there another way to conditionally set the exit-code-sucess-p argument?

[purcell]

While inline lambdas don't work, and that's a bit annoying, you can do something like this:

(defun sqruff--success-p (code) (= 1 code))

(reformatter-define sqruff :program "sqruff" :exit-code-success-p sqruff--success-p)

[purcell]

There probably is a more "correct" way to address this in the macro itself, but it's not obvious to me.

[tah-alv]

Actually, I tried using a lambda as the :exit-code-success-p arg and it worked fine, but I'm having trouble changing the value of the arg based on the value of sqlformat-command. Wouldn't

(reformatter-define sqruff :program "sqruff" :exit-code-success-p sqruff--success-p)

define a new reformatter command specific to sqruff that wouldn't integrate with sqlformat?

I tried to define a function that would return an appropriate predicate function but (unsurprisingly) got the same error as before:

(defun sqlformat--sqruff-success-p (exit-code)
    "Return t if the EXIT-CODE indicates success for sqruff."
    (= exit-code 1))

(defun sqlformat--get-success-p (name)
    "Return a function to check if the exit code indicates success for the command NAME."
        (pcase name
                ('sqruff #'sqlformat--sqruff-success-p)
                (_ #'zerop))
    )

;;;###autoload (autoload 'sqlformat-buffer "sqlformat" nil t)
;;;###autoload (autoload 'sqlformat-region "sqlformat" nil t)
;;;###autoload (autoload 'sqlformat-on-save-mode "sqlformat" nil t)
(reformatter-define sqlformat
    :program (pcase sqlformat-command
                 (`sqlformat "sqlformat")
                 (`sqlfmt "sqlfmt")
                 (`pgformatter "pg_format")
                 (`sqlfluff "sqlfluff")
                 (`sqruff "sqruff")
                 (`sql-formatter "sql-formatter"))
    :args (pcase sqlformat-command
              (`sqlformat  (append sqlformat-args '("-r" "-")))
              (`sqlfmt  (append sqlformat-args '("-")))
              (`pgformatter (append sqlformat-args '("-")))
              (`sqlfluff (append '("format") sqlformat-args '("-")))
              (`sqruff (append sqlformat-args '("fix" "-")))
              (`sql-formatter sqlformat-args))
    :exit-code-success-p (sqlformat--get-success-p sqlformat-command)
    :lighter " SQLFmt"
    :group 'sqlformat)

This might work if reformatter would check to see if :exit-code-success-p was a cons and evaluate it before doing (cl-assert (functionp exit-code-success-p)), but this probably has lots of side effects that wouldn't be worth the change.

[purcell]

Sorry, I missed that this was on sqlformat, and not directly on reformatter. 😁

I think the most direct solution here would be something like this:

diff --git a/sqlformat.el b/sqlformat.el
index d2256f4..0f5507e 100644
--- a/sqlformat.el
+++ b/sqlformat.el
@@ -80,6 +80,13 @@ reformatted SQL to STDOUT, returning an appropriate exit code."
 For example, these args may be useful for sqlformat: (\"-k\" \"upper\")"
   :type '(repeat string))

+
+(defun sqlformat--exit-code-success-p (exit-code)
+  "Return t if the EXIT-CODE indicates success for `sqlformat-command'."
+  (pcase sqlformat-command
+    (`sqruff (= exit-code 1))
+    (_ (zerop exit-code))))
+
 
 ;; Commands for reformatting

@@ -92,13 +99,16 @@ For example, these args may be useful for sqlformat: (\"-k\" \"upper\")"
              (`sqlfmt "sqlfmt")
              (`pgformatter "pg_format")
              (`sqlfluff "sqlfluff")
+             (`sqruff "sqruff")
              (`sql-formatter "sql-formatter"))
   :args (pcase sqlformat-command
           (`sqlformat  (append sqlformat-args '("-r" "-")))
           (`sqlfmt  (append sqlformat-args '("-")))
           (`pgformatter (append sqlformat-args '("-")))
           (`sqlfluff (append '("format") sqlformat-args '("-")))
+          (`sqruff (append sqlformat-args '("fix" "-")))
           (`sql-formatter sqlformat-args))
+  :exit-code-success-p sqlformat--exit-code-success-p
   :lighter " SQLFmt"
   :group 'sqlformat)

[purcell]

I would accept a PR for this, but I'll note that I have mixed feelings about this package growing to support lots of different formatters — that was pretty much exactly what I made reformatter to avoid! The alternative would be for you to make a tiny sqruff-fix.el package using reformatter, then you'll get a sqruff-fix-on-save-mode you can use directly.