[WIP][RFC] Fix broken format for continued comments on GitHub

[rngtng]

This adds splitAtLineBreak which splits comment into a slice with string up to max len separated by join which gets appended to the ends of the middle strings.

If a split contains a single needle, we append it, as well to next middle string to get the count balanced. This ensures the formatting is correct.

It’s for sure not the nicest solution, but parsing markdown or similar felt overhead.

I on purpose didn’t changed splitAtMaxChars in first place for introducing the algo & easier code review, but I guess this can be easily merged into splitAtLineBreak when we accept that a comment with no linebreak is no realise scenario we have to consider properly.

Adds close and (re)open <details> tag in between the split portions to
ensure the markup is correct across multiple comments. This isn't the prettiest, but a close result, under the assumption continued-comments are in 99% within a terraform output anyway...

ref: #111

[rngtng]

hm linter fails, but I dunno why :( pls advice

[lkysow]

Hey thank for the PR! looks like the linter is a bit broken. Don't worry about it. I'll take a look.

[lkysow]

So there's a couple of changes I think we need to make:

1. The needle actually needs to be split into two concepts: the start of the code block and the end. That's because the start of the code block needs to be ```diff\n and the end of the code block needs to be \n```\n.
   • The start needs to have the diff syntax highlighting to make reading the terraform diff easier
   • The end needs to have a newline before and after the ``` otherwise it doesn't format properly.
2. I'd like to remove the function args join and needle and just hardcode them. Then I'd like the tests to be changed accordingly.
   • I want to do this because it's too hard to look at the tests and know that the function is actually working as expected.
3. It's not enough to just count the instances of ```, because what if someone has those characters in their plan output? Instead, we need to count the matches of ^```diff\n and \n```\n to ensure that the code block is actually from the Atlantis comment.

Here are the test cases I was using: https://gist.github.com/lkysow/f161a756fa9cca532493589a0677c338

What do you think?

[lkysow]

Oh also, if you rebase off of master the linter warnings should be fixed.

[rngtng]

hey @lkysow - thanks for you great feedback. Fully agree on all. I should have read the markdown code syntax a bite more carefully :) I hope to get it done by end of the week.. let's see :)

[rngtng]

@lkysow quick update, I'm still on this. That's what I got so far. works, but I feel it's still over-engineered. I ran in the problem to solve it for every special case, but in practise those usecase are rare/unlikely. I'd like to give it another try with more assumptions:

1. content chards >>> needle & continue chars
2. content has no line which is > max_chars
3. first view lines are 'normal text', then the code formatting starts, and it will be closed at the end.
   multiple open & close not happening.

What you think?

btw. by using recursion I found it easier to solve & keep code readable. But as I'm quite with unexperienced with golang, I'm unsure how a high size stack depth is handled. Any recommendation here? Maybe with my assumptions above I can go back to iterative approach with maintaining readability

[lkysow]

Yes I think it would be best to write the algorithm with some assumptions and just throw an error if they're not met. It will make the code much easier to understand and test.

That being said, the assumptions that can be made are:

1. Content chars >>> needle and continue
2. Content has no line which is > max chars

It's incorrect to assume that multiple open and closes won't happen because in a repo with multiple terraform projects, when Atlantis autoplans it includes the plans for each project in a single comment, ex:

---

Atlantis ran plan in 2 projects:

• dir: dir1 workspace: default
• dir: dir2 workspace: default

dir: dir1 workspace: default

plan here

To delete click here...

dir: dir2 workspace: default

plan here

To delete click here...

---

• regarding recursion, don't worry about the stack depth because I can't imagine there being more than 100 comments worth. I think the recursive solution will be more understandable
• I'm finding your test cases pretty hard to understand. Is there a chance you can go with actual strings?

[majormoses]

@rngtng you still working on this? Definitely something I need as well.

[lkysow]

We now wrap comments in an expandable block:

<details><summary>Show Output</summary>

\```diff
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.
\```
</details>

So that's what will need to be closed and re-opened now.

[rngtng]

sorry my silence here, I wish & plan to continue the fix, but I recently received my second child, so focus is currently bit shifted 😄

I'll consider the new format, IMHO this will make things even simpler.. 👍

[sstarcher]

@rngtng do you need any assistance with this?

[rngtng]

@sstarcher honestly: yes, I'd very much appreciate it. I currently just don't find the time to get it done.
Solution seemed straight forward in first place, but the edgecases make it tricky.

After multiple rounds using line split, recursion, regexp etc. I'm now thinking we may keep the current approach but add close and (re)open <details> tag in between the portions. Isn't the prettiest, but a close result - isn't multiple-comment in 99% within a terraform output anyway? This may not be perfect but for sure better than the current state, and yet simple.

Please feel free to take this over if you have a solution in mind.

[rngtng]

Just pushed a commit inserting a close and (re)open <details> tag. See it as a RFC, tagged the PR title accordingly.

[rngtng]

@sstarcher @lkysow any feedback on this?

[sstarcher]

I think a simple close/reopen of the details would be the easiest thing. I have not had a chance to test out your changes.

[lkysow]

@rngtng ack I totally missed your message about adding the <details> change. Sorry. I will check it out next week.

[lkysow]

@rngtng it's merged! I think I will remember this PR as what happens when we try to overengineer something. The algorithm you wrote was pretty cool but I like the simplicity of what you ended up with better. Thanks for sticking with it.

[rngtng]

👍 indeed - you don't know how much this kept bugging me - completeness/correctness vs. simplicity/pragmatism .. I'll take this challenge to my next job candidate interview ;D