fix(formatter): formatting long function calls on multiple lines, fixing formatting of begin nodes in condition

Author: SuperFola

.github/workflows/validate-links.yml

@@ -51,8 +51,18 @@ jobs:
           path: .lycheecache
           key: ${{ steps.restore-cache.outputs.cache-primary-key }}
 
+      - uses: nickderobertis/check-if-issue-exists-action@8edfc8560578e80eb6061565e5359d85d5d63b74
+        name: Check if Issue Exists
+        id: check_if_issue_exists
+        with:
+          repo: ArkScript-lang/Ark
+          token: ${{ secrets.GITHUB_TOKEN }}
+          title: Link Checker Report
+          labels: report, automated issue
+
+      # todo: update existing issue if it already exists
       - name: Create Issue From File
-        if: steps.lychee.outputs.exit_code != 0
+        if: steps.lychee.outputs.exit_code != 0 && steps.check_if_issue_exists.outputs.exists == 'false'
         uses: peter-evans/create-issue-from-file@v5
         with:
           title: Link Checker Report

CHANGELOG.md

@@ -7,8 +7,8 @@
 
 constexpr struct FormatterConfig
 {
-    static constexpr std::size_t SpacePerIndent = 2;   ///< Indentation level of each node
-    static constexpr std::size_t LongLineLength = 32;  ///< Max number of characters per line segment to consider splitting
+    static constexpr std::size_t SpacePerIndent = 2;    ///< Indentation level of each node
+    static constexpr std::size_t LongLineLength = 120;  ///< Max number of characters per line segment to consider splitting
 } FormatterConfig;
 
 class Formatter final
@@ -88,6 +88,8 @@ class Formatter final
      */
     static std::size_t lineOfLastNodeIn(const Ark::internal::Node& node);
 
+    [[nodiscard]] bool isLongLine(const Ark::internal::Node& node);
+
     /**
      * @brief Decide if a node should be split on a newline or not
      * @param node

include/CLI/Formatter.hpp

@@ -8,9 +8,11 @@
 #include <Ark/Error/Exceptions.hpp>
 #include <Ark/Error/Diagnostics.hpp>
 #include <Ark/Compiler/Common.hpp>
+#include <Ark/Utils/Literals.hpp>
 
 using namespace Ark;
 using namespace Ark::internal;
+using namespace Ark::literals;
 
 Formatter::Formatter(const bool dry_run) :
     m_dry_run(dry_run), m_parser(/* debug= */ 0, ParserMode::Raw), m_updated(false)
@@ -134,15 +136,28 @@ std::size_t Formatter::lineOfLastNodeIn(const Node& node)
     return node.position().start.line;
 }
 
-bool Formatter::shouldSplitOnNewline(const Node& node)
+bool Formatter::isLongLine(const Node& node)
 {
     const std::string formatted = format(node, 0, false);
-    const std::string::size_type sz = formatted.find_first_of('\n');
+    const std::size_t max_len =
+        std::ranges::max(
+            Utils::splitString(formatted, '\n'),
+            [](const std::string& lhs, const std::string& rhs) {
+                return lhs.size() < rhs.size();
+            })
+            .size();
+    const std::size_t newlines = std::ranges::count(formatted, '\n');
+
+    // split on multiple lines if we have a very long node,
+    // or if we added many line breaks while doing dumb formatting
+    return max_len >= FormatterConfig::LongLineLength || (newlines > 0 && node.isListLike() && newlines + 1 >= node.constList().size());
+}
 
-    const bool is_long_line = !((sz < FormatterConfig::LongLineLength || (sz == std::string::npos && formatted.size() < FormatterConfig::LongLineLength)));
+bool Formatter::shouldSplitOnNewline(const Node& node)
+{
     if (node.comment().empty() && (isBeginBlock(node) || isFuncCall(node)))
         return false;
-    if (is_long_line || (node.isListLike() && node.constList().size() > 1) || !node.comment().empty())
+    if (isLongLine(node) || (node.isListLike() && node.constList().size() > 1) || !node.comment().empty())
         return true;
     return false;
 }
@@ -302,7 +317,7 @@ std::string Formatter::formatFunction(const Node& node, const std::size_t indent
     {
         bool comment_in_args = false;
         std::string args;
-        const bool split = shouldSplitOnNewline(args_node);
+        const bool split = (isLongLine(args_node) || !args_node.comment().empty());
 
         for (std::size_t i = 0, end = args_node.constList().size(); i < end; ++i)
         {
@@ -354,7 +369,7 @@ std::string Formatter::formatCondition(const Node& node, const std::size_t inden
         cond_on_newline ? "\n" : " ",
         formatted_cond);
 
-    const bool split_then_newline = shouldSplitOnNewline(then_node);
+    const bool split_then_newline = shouldSplitOnNewline(then_node) || isBeginBlock(then_node);
 
     // (if cond then)
     if (node.constList().size() == 3)
@@ -509,6 +524,19 @@ std::string Formatter::formatCall(const Node& node, const std::size_t indent)
     }
 
     std::string result = is_list ? "[" : ("(" + format(node.constList()[0], indent, false));
+
+    // Split args on multiple lines even if, individually, they fit in the configured line length, if grouped together
+    // on a single line they are too long
+    const std::size_t args_line_length = std::accumulate(
+        formatted_args.begin(),
+        formatted_args.end(),
+        result.size() + 1,  // +1 to count the closing paren/bracket
+        [](const std::size_t acc, const std::string& val) {
+            return acc + val.size() + 1_z;
+        });
+    if (args_line_length >= FormatterConfig::LongLineLength)
+        is_multiline = true;
+
     for (std::size_t i = 0, end = formatted_args.size(); i < end; ++i)
     {
         const std::string& formatted_node = formatted_args[i];

lib/std

@@ -9,31 +9,34 @@ using namespace boost;
 ut::suite<"Formatter"> formatter_suite = [] {
     using namespace ut;
 
-    iterTestFiles(
-        "FormatterSuite",
-        [](TestData&& data) {
-            std::string formatted_code;
-
-            should("output a correctly formatted code for " + data.stem) = [&] {
-                Formatter formatter(data.path, /* dry_run= */ true);
-                expect(nothrow([&] {
-                    mut(formatter).run();
-                }));
-
-                formatted_code = formatter.output();
-                // data.expected is ltrim(rtrim(file content))
-                // we want to ensure that a blank line has been added
-                expectOrDiff((data.expected + "\n"), formatted_code);
-            };
-
-            should("not update an already correctly formatted code (" + data.stem + ")") = [&] {
-                Formatter formatter(/* dry_run= */ true);
-                expect(nothrow([&] {
-                    mut(formatter).runWithString(formatted_code);
-                }));
-
-                const std::string code = formatter.output();
-                expectOrDiff(formatted_code, code);
-            };
-        });
+    for (const std::string& subfolder : { "basics", "codeSamples" })
+    {
+        iterTestFiles(
+            "FormatterSuite/" + subfolder,
+            [](TestData&& data) {
+                std::string formatted_code;
+
+                should("output a correctly formatted code for " + data.stem) = [&] {
+                    Formatter formatter(data.path, /* dry_run= */ true);
+                    expect(nothrow([&] {
+                        mut(formatter).run();
+                    }));
+
+                    formatted_code = formatter.output();
+                    // data.expected is ltrim(rtrim(file content))
+                    // we want to ensure that a blank line has been added
+                    expectOrDiff((data.expected + "\n"), formatted_code);
+                };
+
+                should("not update an already correctly formatted code (" + data.stem + ")") = [&] {
+                    Formatter formatter(/* dry_run= */ true);
+                    expect(nothrow([&] {
+                        mut(formatter).runWithString(formatted_code);
+                    }));
+
+                    const std::string code = formatter.output();
+                    expectOrDiff(formatted_code, code);
+                };
+            });
+    }
 };

src/arkscript/Formatter.cpp

@@ -106,7 +106,7 @@ std::string sanitizeRuntimeError(const std::exception& e)
 void expectOrDiff(const std::string& expected, const std::string& received)
 {
     const bool comparison = expected == received;
-    boost::ut::expect(comparison) << [&] {
+    const auto diff = [&] {
         dtl::Diff<std::string, std::vector<std::string>> d(
             Ark::Utils::splitString(received, '\n'),
             Ark::Utils::splitString(expected, '\n'));
@@ -118,4 +118,6 @@ void expectOrDiff(const std::string& expected, const std::string& received)
 
         return stream.str();
     };
+
+    boost::ut::expect(comparison) << diff;
 }

tests/unittests/Suites/FormatterSuite.cpp

@@ -7,4 +7,4 @@
         (list:forEach _listeners (fun (element)
             (if (= typ (@ element 0)) {
                 ((@ element 1) val)
-                (set found true)})))
+                (set found true)})))