kkinnear/zprint

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

eval-on-point opened this issue · 2 comments

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.

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.

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