cider icon indicating copy to clipboard operation
cider copied to clipboard

Setting options to customize cljfmt does not take effect

Open laheadle opened this issue 3 years ago • 10 comments

Expected behavior

Setting the option to customize the behavior of cljfmt should take effect by customizing that behavior.

Actual behavior

Setting the option to customize the behavior of cljfmt has no effect.

Steps to reproduce the problem

I have set:

(setq cider-format-code-options
        '(("remove-consecutive-blank-lines?" t)
          ("remove-multiple-non-indenting-spaces?" t)))

And inside this file I have run cider-format-buffer:

(ns temporale.network.test-format)

(def my-map {:a :b :c      :d})

I would expect this file to be reformatted like this

(ns temporale.network.test-format)

(def my-map {:a :b :c :d})

But instead nothing has changed.

I reported this issue on slack a couple of weeks ago and yuhan replied: This appears to be a bug, the cider--nrepl-format-code-request-map function returns an empty dict

Environment & Version information

CIDER version information

;; CIDER 1.4.0 (Kyiv), nREPL 0.9.0
;; Clojure 1.10.2, Java 1.8.0_332

Emacs version

27.1

Operating system

A pretty recent ubuntu

JDK distribution

openjdk 1.8.0_332

laheadle avatar May 28 '22 12:05 laheadle

Checking out https://github.com/clojure-emacs/cider/blob/0f8fd4d84decfa3514fbdd556c5235f490b7a507/cider-client.el#L247-L272, it seems like it only will relay indents and alias-map options.

vemv avatar May 28 '22 12:05 vemv

Indeed. I don't remember the background, but I'd assume that probably we just weren't very familiar with the options. I know that certainly applies to me. :-)

bbatsov avatar May 31 '22 06:05 bbatsov

This can be partially fixed by merging the format-options into the dict sent to nrepl, but unfortunately there are upstream issues in nrepl (see https://github.com/nrepl/nrepl/discussions/275#discussioncomment-2790176) which make certain options unrepresentable - eg. attempting to set any default-true option to false.

Eg. writing {:indentation? false} in elisp as '(("indentation?" nil)) sends an empty vector, I tried 0 or 'false or "false" and none of them worked.

Edit: the partial fix-

(defun cider--nrepl-format-code-request-map (&optional format-options)
  "Map to merge into requests that require code formatting.
If non-nil, FORMAT-OPTIONS specifies the options cljfmt will use to format
the code.  See `cider-format-code-options` for details."
  (when format-options
    (let* ((indents-dict (when (assoc "indents" format-options)
                           (thread-last
                             (cadr (assoc "indents" format-options))
                             (map-pairs)
                             (seq-mapcat #'identity)
                             (apply #'nrepl-dict))))
           (alias-map-dict (when (assoc "alias-map" format-options)
                             (thread-last
                               (cadr (assoc "alias-map" format-options))
                               (map-pairs)
                               (seq-mapcat #'identity)
                               (apply #'nrepl-dict)))))
      (thread-last
        (map-merge 'list
+                  format-options
                   (when indents-dict
                     `(("indents" ,indents-dict)))
                   (when alias-map-dict
                     `(("alias-map" ,alias-map-dict))))
        (map-pairs)
        (seq-mapcat #'identity)
        (apply #'nrepl-dict)))))

yuhan0 avatar Jun 01 '22 10:06 yuhan0

The false thing could be fixed as in refactor-nrepl https://github.com/clojure-emacs/refactor-nrepl/blob/c2f629d82879e12c8976c23b68008bacce72a68b/src/refactor_nrepl/config.clj#L52-L54 i.e. it would need explicit support in cider-nrepl.

Although @bbatsov wasn't a fan of this technique.

vemv avatar Jun 01 '22 11:06 vemv

Honestly I think that a much cleaner API would be to let users write cljfmt config in .edn format, and honor those files when found. It's what clojure-lsp does, for instance.

vemv avatar Jun 01 '22 11:06 vemv

Would these .edn files then be sent over the wire as bencode strings for cider-nrepl to parse back into Clojure structures? The special casing of "true" / "false" strings in each middleware does seem like an ugly hack.. perhaps there should be some sort of reserved keywords that allow for decoding at a lower level.

I hoped that cljfmt could recognize its own config file, allowing us to sidestep the issue in this case, but it seems that is not implemented https://github.com/weavejester/cljfmt/issues/256

yuhan0 avatar Jun 01 '22 11:06 yuhan0

Would these .edn files then be sent over the wire as bencode strings for cider-nrepl to parse back into Clojure structures?

No, cider-nrepl would just read the resource, bypassing any complexity

Here's clojure-lsp's pattern https://github.com/clojure-lsp/clojure-lsp/blob/40b6e70fd4f529d6fdf0b2ec02f11045ab7bbcfd/.cljfmt.edn

The .cljfmt.edn naming is probably 'unofficial' but I do think it would be a good idea to detect that exact file.

That way we would facilitate usages by a variety of users - nobody enjoys duplicated config

vemv avatar Jun 01 '22 11:06 vemv

I'm open to ideas for improving this. Admittedly this functionality never got enough love, mostly because relatively few people use it.

bbatsov avatar Jun 01 '22 12:06 bbatsov