search

kkinnear / zprint

Executables, uberjar, and library to beautifully format Clojure and Clojurescript source code and s-expressions.
MIT License
547 stars 46 forks source link

Make minimum number of map entries `lift-ns?` requires configurable #325

Open eval-on-point opened 2 weeks ago

eval-on-point commented 2 weeks ago

From the changelog:

{:map {:lift-ns? false}} -- the default for :map :lift-ns? was changed to false from true, largely because formatting deps.edn files looks ugly when the :mvn is lifted out. An alternative would have been to only lift a namespace out of a map if there were more than one key. Please submit an issue if you feel strongly about this one way or the other and we can discuss it.

I was discussing the lift-ns? option with my team this morning and we all (N=3) agreed that we would prefer namespaces to be lifted if there were more than one key in a map, but not to lift it if there were only one. We thought it would be slick if lift-ns? could optionally take in integer argument instead of true. This would set the minimum number of map elements required before zprint performs a namespace lift in a backwards compatible way.

kkinnear commented 1 week ago

Many thanks for your suggestion! I think it is a reasonable approach, though it hurts a bit to put something in addition to true and false an option for a key ending in ?. Still, I don't really think we need yet another key-value pair here, so I'll see what I can to do to make this work. I'll let you know when I've figured it out and finished it.

eval-on-point commented 1 week ago

That is an interesting philosophical point and is possibly evidence that ? should be reserved only for boolean-valued predicates. Check out https://github.com/bbatsov/clojure-style-guide/issues/182#issuecomment-516303299 for an interesting perspective on this. I am not exactly sure where I land.