Fix #45: Thread first changes the indentation of the inner forms

[lambdank]

Here is an attempt to solve issue #45.

The concrete issue is nesting a -> with a cond-> inside. Because cond-> has indent [[:block 1]], the parameters will not be indented in pairs as intended.
As mentioned in this comment, the indentation of an inner form within a -> does not take the threading into account.

The proposed solution is to take the parent node into account, when determining the index of the indentation of the current node.
The check is:

• Is the parent form a thread-first macro?
• Apart from this, there is the special case, which is the first argument to a thread-first, which should be indented as usual.

I just made this fix for thread-first macros in Clojure core. We could maybe do a regex instead?

Please let me know, if I need to do any documentation or something else is missing.

[weavejester]

Thanks for the PR. This obviously needs to be configurable before it can be merged. Ideally this would happen through an indentation rule, e.g. :thread to complement :block, though that might be a little tricky.

I think I need to take another look at the indentation logic anyway. I don't have a lot of time over the next couple of weeks, but I'll see if I can squeeze in a few hours to go over the indentation system.

[weavejester]

Instead of adjust-idx which doesn't convey why the index is being adjusted, perhaps instead we just split out the predicate:

(defn- in-thread-macro? [zloc context]
  (and (form-matches-thread-macro? zloc) (not (first-argument? zloc))))

Then we could write:

(cond-> idx (in-thread-macro? zloc context) dec)

I'm unsure why you have (some-> dec (max 0)) instead of just dec. Under what circumstances would the index be less than or equal to zero?

[lambdank]

Wouldn't idx be zero in this case, if I configured {cond-> [[:block 0]]}?
There is a check for (nil? idx) in inner-indent already. Isn't it nil when there is no indentation rule, like (-> x foo bar)?

[weavejester]

The index is the position of the child in the parent form, IIRC. So, come to thing about it, we could change (not (first-argument? zloc)) to (> idx 1).

[lambdank]

Doesn't the index come from the :indents as specified here?
So for :block it uses default indenting until we reach the index. So if we are in a thread, we should basically hit that index one spot before (ie. decrease the index). If the index is 0, we should keep it at 0.
For :inner when index is set, it is restricted to that particular index. So that should also occur one spot earlier (ie. decrease the index). If the index is 0, then we should avoid using this indentation rule, since the spot it was meant for is not present.

I also oversaw the depth argument of inner. Here we should check whether the parent of the top zloc is wrapped in a thread. I'll commit a fix.

Hope it makes sense.

[weavejester]

Ah, yes, I believe you're right. I need to refresh myself on the indentation logic as it's been a while since I've looked at it.

[lambdank]

Thanks for the PR. This obviously needs to be configurable before it can be merged. Ideally this would happen through an indentation rule, e.g. :thread to complement :block, though that might be a little tricky.

I think I need to take another look at the indentation logic anyway. I don't have a lot of time over the next couple of weeks, but I'll see if I can squeeze in a few hours to go over the indentation system.

As I see it, the tricky part about having a :thread indentation rule is, that it applies to the children and not to the node itself.
I could do something where I create a :thread rule, just indent them with the default indentation and gather them in the context to a use them as a replacement for the hardcoded #{'-> 'as-> 'some-> 'cond->}?

[weavejester]

What about doing it this way:

(defn- form-symbol-matches? [zloc pred context]
  (let [possible-sym (form-symbol zloc)]
    (or (pred (fully-qualified-symbol possible-sym context))
        (pred (remove-namespace possible-sym)))))

(defn- form-matches-key? [zloc key context]
  (form-symbol-matches? zloc #(symbol-matches-key? % key) context))

(defn- form-matches-thread-macro? [zloc context]
  (form-symbol-matches? zloc #{'-> 'as-> 'some-> 'cond->} context))

[weavejester]

As I see it, the tricky part about having a :thread indentation rule is, that it applies to the children and not to the node itself. I could do something where I create a :thread rule, just indent them with the default indentation and gather them in the context to a use them as a replacement for the hardcoded #{'-> 'as-> 'some-> 'cond->}?

Yes, from a user perspective, we'd expect indentation rules to be all in one place, but threaded indentation is tricky because it depends on both the indentation rules for the current form and the indentation rules for the outer form.

It gets even trickier when we consider a form like cond->, where only arguments 3, 5, 7, etc. are threaded. I'm unsure of a good way to express that as a rule, unless we add in meta-indexes like :even and :odd to complement integer indexes.

[lambdank]

All right, I thought a bit about it and gave it another shot.
Just to sum up the changes in the last commit:

• I introduced a new rule called :thread. As can be seen in the indenter-fn defmethod, it does not directly influence the form itself, but rather influences the child forms. Therefore, I gather them in the context. This rule can have take an idx, which correspondes to at which argument the threading starts (eg. for as-> it's 3). This can also be :even or :odd, where it maches every other form. Here the idx is hardcoded to 2 (which might not be necessary, coming to think of it).
• Most of the complexity lies in in-thread-macro?. Before we should just look at the node and it's parent, but since we now allow even/odd, we also need to check, whether the parent is in a threading macro. If it is, we raise the position by one, such that it reflects the "actual" position. This means that this is a recursive function, which stops when the parent is the root or not in a threading macro.

I haven't written any documentation yet, since I wanted to hear your take on the approach @weavejester, before doing that work. Let me now what you think.

[lambdank]

I hope this test (the last one) might reflect the recursion:

(is (reformats-to?
         ["(cond-> a"
          "(cond-> 1
                  odd? inc) 
          (cond-> a b 
                  c d))"]
         ["(cond-> a"
          "  (cond-> 1"
          "    odd? inc)"
          "  (cond-> a b"
          "          c d))"]))

[weavejester]

I think I need to give this some thought on how best to approach this. The abstraction for how indentation is applied needs to be changed somehow. Apologies for being slow on this; I haven't had a lot of time to delve into it deeply.

[weavejester]

How does this differ from index-of?

[weavejester]

I think we need to rethink how the indentation functions are applied, as the abstraction has clearly failed if we have an indenter function that does nothing.

[lambdank]

No worries, I postponed this fix for several weeks.

I also thought about an approach, where the indentation happens by going through the AST recursively.
As far as I can see, the indentation is only neighbors and parents, but not children. In a simplistic view, the node is also just impacted by nodes before, but that limits stuff like pair-alignment. This might be fixable by doing some sort of reverse-sweep with the context.
I just wanted to fix this issue with the least intrusive solution.

Feel free to reach out, if you get time at some point and envision a solution that I can help with.