From 5028223c37a3bedeaa9ec39b6334e12a00815d2f Mon Sep 17 00:00:00 2001 From: Andrew Kaster Date: Tue, 25 Jul 2023 11:52:06 -0600 Subject: [PATCH] Meta+Utilities: Make pre-commit checks significantly less verbose When markdown-check is built, it outputs hundreds of lines of "ignoring this and that link because reasons". This is extremely not helpful when trying to figure out exactly which check failed on your commit. Also remove the timing numbers from lint-ci.sh These are just noise and also don't help to figure out which pre-commit check failed. Ideally the output on fail should be "[OK]: Check A" for all the passing checks and "[FAIL] Check N" with the required context for the failed check. --- Meta/lint-ci.sh | 12 ++++------- Userland/Utilities/markdown-check.cpp | 29 +++++++++++++++++---------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/Meta/lint-ci.sh b/Meta/lint-ci.sh index e60aa774f3..de8129ff3b 100755 --- a/Meta/lint-ci.sh +++ b/Meta/lint-ci.sh @@ -34,8 +34,7 @@ for cmd in \ Meta/lint-prettier.sh \ Meta/lint-python.sh \ Meta/lint-shell-scripts.sh; do - echo "Running ${cmd}" - if time "${cmd}" "$@"; then + if "${cmd}" "$@"; then echo -e "[${GREEN}OK${NC}]: ${cmd}" else echo -e "[${RED}FAIL${NC}]: ${cmd}" @@ -44,8 +43,7 @@ for cmd in \ done if [ -x ./Build/lagom/bin/IPCMagicLinter ]; then - echo "Running IPCMagicLinter" - if time { git ls-files '*.ipc' | xargs ./Build/lagom/bin/IPCMagicLinter; }; then + if { git ls-files '*.ipc' | xargs ./Build/lagom/bin/IPCMagicLinter; }; then echo -e "[${GREEN}OK${NC}]: IPCMagicLinter (in Meta/lint-ci.sh)" else echo -e "[${RED}FAIL${NC}]: IPCMagicLinter (in Meta/lint-ci.sh)" @@ -55,8 +53,7 @@ else echo -e "[${GREEN}SKIP${NC}]: IPCMagicLinter (in Meta/lint-ci.sh)" fi -echo "Running Meta/lint-clang-format.sh" -if time Meta/lint-clang-format.sh --overwrite-inplace "$@" && git diff --exit-code; then +if Meta/lint-clang-format.sh --overwrite-inplace "$@" && git diff --exit-code; then echo -e "[${GREEN}OK${NC}]: Meta/lint-clang-format.sh" else echo -e "[${RED}FAIL${NC}]: Meta/lint-clang-format.sh" @@ -70,8 +67,7 @@ fi # when Ports/ files have changed and only invoke lint-ports.py when needed. # if [ "$ports" = true ]; then - echo "Running Meta/lint-ports.py" - if time Meta/lint-ports.py; then + if Meta/lint-ports.py; then echo -e "[${GREEN}OK${NC}]: Meta/lint-ports.py" else echo -e "[${RED}FAIL${NC}]: Meta/lint-ports.py" diff --git a/Userland/Utilities/markdown-check.cpp b/Userland/Utilities/markdown-check.cpp index 897d6d4c25..7662ffcfae 100644 --- a/Userland/Utilities/markdown-check.cpp +++ b/Userland/Utilities/markdown-check.cpp @@ -72,7 +72,7 @@ class MarkdownLinkage final : Markdown::Visitor { public: ~MarkdownLinkage() = default; - static MarkdownLinkage analyze(Markdown::Document const&); + static MarkdownLinkage analyze(Markdown::Document const&, bool verbose); bool has_anchor(DeprecatedString const& anchor) const { return m_anchors.contains(anchor); } HashTable const& anchors() const { return m_anchors; } @@ -80,7 +80,8 @@ public: Vector const& file_links() const { return m_file_links; } private: - MarkdownLinkage() + MarkdownLinkage(bool verbose) + : m_verbose(verbose) { auto const* source_directory = getenv("SERENITY_SOURCE_DIR"); if (source_directory != nullptr) { @@ -96,13 +97,14 @@ private: HashTable m_anchors; Vector m_file_links; bool m_has_invalid_link { false }; + bool m_verbose { false }; DeprecatedString m_serenity_source_directory; }; -MarkdownLinkage MarkdownLinkage::analyze(Markdown::Document const& document) +MarkdownLinkage MarkdownLinkage::analyze(Markdown::Document const& document, bool verbose) { - MarkdownLinkage linkage; + MarkdownLinkage linkage(verbose); document.walk(linkage); @@ -184,7 +186,8 @@ RecursionDecision MarkdownLinkage::visit(Markdown::Text::LinkNode const& link_no auto url = URL::create_with_url_or_path(href); if (url.is_valid()) { if (url.scheme() == "https" || url.scheme() == "http") { - outln("Not checking external link {}", href); + if (m_verbose) + outln("Not checking external link {}", href); return RecursionDecision::Recurse; } if (url.scheme() == "help") { @@ -224,7 +227,7 @@ RecursionDecision MarkdownLinkage::visit(Markdown::Text::LinkNode const& link_no warnln("Binary link named '{}' is not allowed, binary links must be called 'Open'. Linked binary: {}", link_text, href); m_has_invalid_link = true; } - } else { + } else if (m_verbose) { outln("Not checking local link {}", href); } return RecursionDecision::Recurse; @@ -287,13 +290,16 @@ ErrorOr serenity_main(Main::Arguments arguments) Core::ArgsParser args_parser; Vector file_paths; bool output_link_graph { false }; + bool verbose_output { false }; StringView base_path = "/"sv; args_parser.add_positional_argument(file_paths, "Path to markdown files to read and parse", "paths", Core::ArgsParser::Required::Yes); args_parser.add_option(base_path, "System base path (default: \"/\")", "base", 'b', "path"); args_parser.add_option(output_link_graph, "Output a page link graph into \"manpage-links.gv\". The recommended tool to process this graph is `fdp`.", "link-graph", 'g'); + args_parser.add_option(verbose_output, "Print extra information about skipped links", "verbose", 'v'); args_parser.parse(arguments); - outln("Reading and parsing Markdown files ..."); + if (verbose_output) + outln("Reading and parsing Markdown files ..."); HashMap files; for (auto path : file_paths) { auto file_or_error = Core::File::open(path, Core::File::OpenMode::Read); @@ -319,10 +325,11 @@ ErrorOr serenity_main(Main::Arguments arguments) // Since this should never happen anyway, fail early. return 1; } - files.set(TRY(FileSystem::real_path(path)), MarkdownLinkage::analyze(*document)); + files.set(TRY(FileSystem::real_path(path)), MarkdownLinkage::analyze(*document, verbose_output)); } - outln("Checking links ..."); + if (verbose_output) + outln("Checking links ..."); bool any_problems = false; for (auto const& file_item : files) { if (file_item.value.has_invalid_link()) { @@ -425,8 +432,8 @@ ErrorOr serenity_main(Main::Arguments arguments) if (any_problems) { outln("Done. Some errors were encountered, please check above log."); return 1; - } else { + } else if (verbose_output) { outln("Done. No problems detected."); - return 0; } + return 0; }