Fix alignment issue involving margin comments.

[samwagg]

Alignment wasn't working properly when nested, multi-line sexps were followed by margin comments.

• The commits are consistent with our contribution guidelines.
• You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
• You've run M-x checkdoc and fixed any warnings in the code you've written.
• You've updated the changelog (if adding/changing user-visible functionality).
• You've updated the readme (if adding/changing user-visible functionality).

Thanks!

[vemv]

This seems an odd example, I really don't recall ever seeing an aligned function invocation.

Hashmaps, cases, etc are more frequent targets.

Also, this piece of code would look exactly the same for the aligned and not aligned cases, so one can't really tell if you're affecting alignment at all with the proposed changes.

I'd suggest choosing a variety of examples, and also, add ; comments at different positions, to get really good coverage.

[samwagg]

I think alignment of let bindings is common. I made the strange decision to use keywords for the left-hand side of the bindings, which I will definitely update.

I will update to add examples for hashmap and case. I plan to still use a single test with a single form, but I will add all three cases within the form. I think that's what you have in mind.

I will also update so that the alignment isn't trivial; I can see how that might be confusing, but I also what to make clear that it is besides the point. The point is that when you align code that is already aligned correctly, it is getting mangled (see the attached issue). This test does fail if you undo the regexp change I made.

[vemv]

Thanks! Appreciated.

Simply for record, I still don't see a particular alignment in the example, neither for the let itself or the function invocations:

I'm aware that let alignment is a thing but it's better shown e.g. here:

...Just in case, indentation != alignment.

btw, this initial newline I added in both of my examples would make the tests more readable. I think it would be welcome if you can update all examples to include it.

(I haven't tried running the tests like that, might require removing the initial newline programatically if present)

[samwagg]

Alright, updated. A couple notes:

• I left the commits unsquashed for now in case it helps with reviewing. But I understand they need to be squashed before merge.
• I had interpreted your request to "update all examples" to include the initial newline as a request to update all the existing indentation tests. But now I'm wondering if you just meant for me to do it for my multiple examples. It seems right to be consistent, but I can easily drop that commit if desired.

[vemv]

Looks all good to me! Great work.

As one last suggestion, you could add examples with ;; comments for greater coverage.

---

I cannot further review these days for personal reasons, someone else will have to approve/merge

[bbatsov]

I'll take it from here.

[samwagg]

I wasn't sure if I was meant to test margin comments that begin ;; or comments on their own line, so I added coverage for both. The latter is distinct from the issue I was testing, imo, so I added a separate test for it. I checked, and it didn't look like there were any existing tests for alignment of sexps with interleaved line comments.

[samwagg]

I wasn't sure if I was meant to test margin comments that begin ;; or comments on their own line, so I added coverage for both. The latter is distinct from the issue I was testing, imo, so I added a separate test for it. I checked, and it didn't look like there were any existing tests for alignment of sexps with interleaved line comments.

Actually, I suppose having one test with a variety of different comment examples makes sense. I guess I just have tunnel vision for the specific bug I am fixing. I can combine the two tests into one that covers all examples if that style is preferred.

[bbatsov]

More test coverage is usually a good thing, plus some conventions are not as strong with Clojure as with older Lisps - e.g. a lot of people use ;; comments everywhere, although historically ;; was a line comment and ; was a margin comment.

[samwagg]

Alright, I think I'm done then. I squashed my commits.

[samwagg]

Hi, just dropping a reminder that I am finished making changes with this PR and I think it's ready. Please let me know if there are any further changes desired.

[bbatsov]

Thanks!

[vemv]

Released as 5.14.0