Prioritise TLDR part over the tree diff

[wzieba]

Description

This PR changes logic of priorities when preparing the "dependency diff commit".

Imagine a PR with huge project dependencies diff and huge build dependencies diff:

• tldr project deps: 1000 chars
• tree project deps: 40 000 chars
• tldr build deps: 500 chars
• tree build deps: 50 000 chars

Limit is 60 000 chars (for simplicity, the real one is 65 400)

Previously:

Priorities were the same as order of printing. So we would print comment like:

• ✅ TLDR list of project deps (1000 chars)
• ✅ tree of project deps (40 000 chars)
• ❌ TLDR list of build deps
• ❌ tree of build deps (because they both had 41 000 chars, which combining with build dependencies: 41 000 + 50 000 + 500 = 91 500 chars is above limit of the comment PR)

Now:

TLDR; lists are prioritised. Trees are only an addition "if space allows"

• ✅ TLDR list of project deps (1000 chars)
• ✅ tree of project deps (40 000 chars)
• ✅ TLDR list of build deps (500 chars)
• ❌ tree of build deps

Testing

Please run bats tests/test_craft_comment_with_dependency_diff.bats - it contains two tests proving that the logic works as intended.

Also see these PRs:

• https://github.com/Automattic/Matticspace-Mobile/pull/119 (all fits, small diff)

• [DNM] testing new comments diff pocket-casts-android#3637 (huge project deps diff, as pocket casts has many modules)

• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[dangermattic]

	1 Warning
⚠️	Please add an entry in the CHANGELOG.md file to describe the changes made by this PR

Generated by 🚫 Danger

[wzieba]

I don't know how to address this 😖

[AliSoftware]

I think I found a solution for that: create helper functions that would do each of the printf calls, that way you can get rid of your template env vars and instead pass the template strings hardcoded directly in those helper functions, thus making shellcheck stop complaining.

Here's a diff I tried for that that seems to work, feel free to can apply it to your PR if you like it:

diff --git a/bin/craft_comment_with_dependency_diff b/bin/craft_comment_with_dependency_diff
index d1d4777..4086ac1 100755
--- a/bin/craft_comment_with_dependency_diff
+++ b/bin/craft_comment_with_dependency_diff
@@ -1,5 +1,5 @@
 #!/bin/bash
-# shellcheck disable=SC2059
 
 # This script is used to craft a comment with a dependency diff and a build environment diff.
 # It's extracted from comment_with_dependency_diff to be tested in isolation.
@@ -11,23 +11,34 @@ MAX_COMMENT_SIZE=65400
 current_size=0
 
 # Templates for content formatting
-SECTION_HEADER="## %s changes\n\n"
-TLDR_TEMPLATE="<details><summary>list</summary>
+print_header() {
+    printf "## %s changes\n\n" "$1"
+}
+
+print_tldr() {
+    printf "<details><summary>list</summary>
 
 \`\`\`
 %s
 \`\`\`
 
 </details>
-"
-TREE_TEMPLATE="<details><summary>tree</summary>
+" "$1"
+}
+
+print_tree() {
+    printf "<details><summary>tree</summary>
 
 \`\`\`diff
 %s
 \`\`\`
 
-</details>"
-TREE_TOO_LARGE="\n ⚠️ %s tree is too large. [View it in Buildkite artifacts](%s#%s)\n"
+</details>" "$1"
+}
+
+print_tree_too_large() {
+    printf "\n ⚠️ %s tree is too large. [View it in Buildkite artifacts](%s#%s)\n" "$1" "$2" "$3"
+}
 
 # Calculate total size needed for TLDRs
 # This assumes that TLDRs are never too large to fit in the comment
@@ -37,7 +48,8 @@ for section in "Project dependencies" "Build environment"; do
     [[ $section == "Build environment" ]] && tldr_file="$TLDR_DIFF_BUILD_ENV_FILE"
     
     if [ -s "$tldr_file" ]; then
-        content=$(printf "$SECTION_HEADER$TLDR_TEMPLATE" "$section" "$(cat "$tldr_file")")
+        # content=$(printf "$SECTION_HEADER$TLDR_TEMPLATE" "$section" "$(cat "$tldr_file")")
+        content=$(print_header "$section"; print_tldr "$(cat "$tldr_file")")
         tldr_size=$(( tldr_size + ${#content} ))
     fi
 done
@@ -59,19 +71,19 @@ for section in "Project dependencies" "Build environment"; do
     
     if [ -s "$tldr_file" ]; then
         # Print section header and TLDR
-        printf "$SECTION_HEADER" "$section" >> "$COMMENT_FILE"
-        printf "$TLDR_TEMPLATE" "$(cat "$tldr_file")" >> "$COMMENT_FILE"
+        print_header "$section" >> "$COMMENT_FILE"
+        print_tldr "$(cat "$tldr_file")" >> "$COMMENT_FILE"
         current_size=$(wc -c < "$COMMENT_FILE")
         
         # Try to add tree if it exists
         if [ -s "$tree_file" ]; then
             tree_content=$(cat "$tree_file")
-            content=$(printf "$TREE_TEMPLATE" "$tree_content")
+            content=$(print_tree "$tree_content")
             if would_content_fit ${#content}; then
                 echo -e "$content" >> "$COMMENT_FILE"
                 current_size=$(wc -c < "$COMMENT_FILE")
             else
-                printf "$TREE_TOO_LARGE" "$section" "$BUILDKITE_BUILD_URL" "$BUILDKITE_JOB_ID" >> "$COMMENT_FILE"
+                print_tree_too_large "$section" "$BUILDKITE_BUILD_URL" "$BUILDKITE_JOB_ID" >> "$COMMENT_FILE"
             fi
         fi
     fi

[wzieba]

This is smart, thank you a lot 🙏

[wzieba]

hi @AliSoftware 👋 friendly reminder about this, do you think we can merge this?