1.2.5 format changes: `dissoc`

[NoahTheDuke]

Hey Kim!

In this commit, the formatting for dissoc was changed. It seems there wasn't anything specific for it previously, so I don't know how it decided to format (hang I guess?).

Here's an example of the change:

;; Given
(dissoc x :aaaaaaaaaaaaaa :bbbbbbbbbb :ccccccccccccccccc :dddddddddddddddddddddd)

; 1.2.4 output
(dissoc x
        :aaaaaaaaaaaaaa :bbbbbbbbbb
        :ccccccccccccccccc :dddddddddddddddddddddd)

; 1.2.5
(dissoc x
 :aaaaaaaaaaaaaa
 :bbbbbbbbbb
 :ccccccccccccccccc
 :dddddddddddddddddddddd)

Worth noting that assoc didn't receive any likewise changes:

; Given
(assoc x :aaaaaaaaaaaaaa 1 :bbbbbbbbbb 2 :ccccccccccccccccc 3 :dddddddddddddddddddddd 4)

; 1.2.4 and 1.2.5
(assoc x
       :aaaaaaaaaaaaaa 1
       :bbbbbbbbbb 2
       :ccccccccccccccccc 3
       :dddddddddddddddddddddd 4)

I don't necessarily think the change is bad but it has added a bunch of changes to our codebase when updating to 1.2.5.

Ideally this wouldn't have changed, but I can add "dissoc" :none to our :fn-map if reverting has implications elsewhere.

Thanks so much

[kkinnear]

Noah -- thanks for asking!

Yes, I added disssoc to the :fn-map because, as you say, there had been nothing there for it previously. So what happened was that the capability I call "constant pairing" came into play, and caused multiple keywords to pair up. As you have shown in your example. Since the keywords in dissoc don't actually have any relationship to each other, pairing them up seemed to imply something that was misleading. They are paired up in assoc because they are key-value pairs. To prevent this pairing, I put dissoc in the :fn-map and turned off constant pairing at the first level. I also made it :arg1 since its first argument was special, like assoc.

You can certainly revert that behavior by, as you note, setting the fn-map for dissoc to :none, and it will go back to the way it was. If you really don't like the :arg1, I would recommend, however, that you replace :arg1 with :none in the :fn-map, but leave the constant pairing turned off. You can't actually do that directly, you would need to replace what's there with

[:none {:list {:constant-pair? false}, :next-inner-restore [[:list :constant-pair?]]}]

That way you won't get the constant pairing. Unless you really like that for some reason?

Anyway, that was my thinking on this change. It is always hard to decide to change some default value, but in this case I felt that it was really misleading people, and that is exactly the opposite of my goals.

[NoahTheDuke]

Cool, that makes a lot more sense, thanks. Maybe noting that in the changelog will prevent further confusion.

I think our goals are at cross purposes, which is understandable. Yours is to build a general tool for the community that is semantic and beautiful and mine is to change as little as possible in my company's codebase when updating libraries. I agree that the way dissoc worked pre-1.2.5 is semantically misleading and shouldn't have been that way, but with a team of 50, adding ~10 extra files of changes increases the probability of merge conflicts and issues down the line. (This is especially relevant with the tuning changes. I'm using :original-tuning even tho it's objectively worse because otherwise, the diff goes from 96 files touched to 371.)

I'm not saying you have to be as conservative as the clojure.core team, just want to highlight why someone might want to keep things that are currently working as close as possible when updating.

Thanks as always for your work on zprint.

[kkinnear]

Thanks for the comment. I've been in exactly your position (though I'm not at present) and I totally understand. That's why I try to always give people a fallback to the "old way" -- when I can. Thanks for reminding me of the consequences of my changes. It is always good to keep an eye on the pain I'm causing people!