From 94a7805ee65d44148998b4d2bb644dee18273a75 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 00:16:10 +0000 Subject: [PATCH 01/18] Add simple util to gather doxygen warnings --- .githooks/check-docs | 41 +++++++++++++++++++++++++++++++++++++++++ CMake/Docs.cmake | 3 ++- Doxyfile | 18 +++++++++--------- 3 files changed, 52 insertions(+), 10 deletions(-) create mode 100755 .githooks/check-docs diff --git a/.githooks/check-docs b/.githooks/check-docs new file mode 100755 index 000000000..dfc8cb7fe --- /dev/null +++ b/.githooks/check-docs @@ -0,0 +1,41 @@ +#!/bin/bash + +# Note: This script is intended to be run from the root of the repository. +# +# Not really a hook but should be used to check the completness of documentation for added code, otherwise CI will come for you. +# It's good to have /tmp as the output so that consecutive runs are fast but no clutter in the repository. + +ROOT=$(pwd) +DOXYGEN=$(command -v doxygen) +TMPDIR=/tmp +TMPFILE=${TMPDIR}/docs.log +DOCDIR=${TMPDIR}/out + +if [[ ! -z "$DOXYGEN" ]]; then + mkdir -p ${DOCDIR} > /dev/null + pushd ${DOCDIR} > /dev/null + + sed -e "s!\${SOURCE}!${ROOT}!" ${ROOT}/Doxyfile > ./Doxyfile + ${DOXYGEN} ./Doxyfile 2> ${TMPFILE} 1> /dev/null + + # We don't want to check for default values and typedefs as well as for member variables + OUT=$(cat ${TMPFILE} | grep -v "=default" | grep -v "\(variable\)" | grep -v "\(typedef\)") + popd > /dev/null + + if [[ ! -z "$OUT" ]]; then + cat < Date: Sat, 2 Mar 2024 00:19:40 +0000 Subject: [PATCH 02/18] Add check_docs job --- .github/workflows/build.yml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index aea5c1e50..d75f6df8c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -19,6 +19,19 @@ jobs: run: | ./.githooks/pre-commit shell: bash + + check_docs: + name: Check documentation + runs-on: ubuntu-20.04 + container: + image: rippleci/clio_ci:latest + steps: + - uses: actions/checkout@v4 + - name: Run linter + id: run_linter + run: | + ./.githooks/check-docs + shell: bash build: name: Build From 8dd966a9f16496d1525abf54b73c886a7dc40bc6 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 00:22:05 +0000 Subject: [PATCH 03/18] Try with dot disabled --- Doxyfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doxyfile b/Doxyfile index 5f4010ef4..ac5a7c3b8 100644 --- a/Doxyfile +++ b/Doxyfile @@ -15,7 +15,7 @@ SORT_MEMBERS_CTORS_1ST = YES INPUT = ${SOURCE}/src EXCLUDE_SYMBOLS = impl RECURSIVE = YES -HAVE_DOT = YES +HAVE_DOT = NO QUIET = YES WARNINGS = YES From 12c3fa9627ac0b2bc3fc3ee8175ae3d893c1ab5e Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 00:24:01 +0000 Subject: [PATCH 04/18] Introduce a doc issue --- src/data/cassandra/Handle.hpp | 2 -- src/util/LedgerUtils.hpp | 4 ++-- src/util/requests/RequestBuilder.hpp | 22 +++++++++++----------- src/util/requests/WsConnection.hpp | 6 +++--- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/data/cassandra/Handle.hpp b/src/data/cassandra/Handle.hpp index 6279a9800..ab879f47b 100644 --- a/src/data/cassandra/Handle.hpp +++ b/src/data/cassandra/Handle.hpp @@ -58,8 +58,6 @@ class Handle { /** * @brief Construct a new handle from a Settings object. - * - * @param clusterSettings The settings to use */ explicit Handle(Settings clusterSettings = Settings::defaultSettings()); diff --git a/src/util/LedgerUtils.hpp b/src/util/LedgerUtils.hpp index 2a1e091a5..fcc8999c2 100644 --- a/src/util/LedgerUtils.hpp +++ b/src/util/LedgerUtils.hpp @@ -36,7 +36,7 @@ namespace util { /** * @brief Returns a string set of all supported ledger entry types * - * @return const& The set of ledger entry types + * @return The set of ledger entry types */ std::unordered_set const& getLedgerEntryTypeStrs(); @@ -53,7 +53,7 @@ getLedgerEntryTypeFromStr(std::string const& entryName); /** * @brief Return the list of ledger entry types which will block the account deletion * - * @return const& The list of ledger entry types + * @return The list of ledger entry types */ std::vector const& getDeletionBlockerLedgerTypes(); diff --git a/src/util/requests/RequestBuilder.hpp b/src/util/requests/RequestBuilder.hpp index c4bdfd0bf..d90b6cd20 100644 --- a/src/util/requests/RequestBuilder.hpp +++ b/src/util/requests/RequestBuilder.hpp @@ -62,7 +62,7 @@ class RequestBuilder { * @brief Add a header to the request * * @param header header to add - * @return reference to itself + * @return Reference to itself */ RequestBuilder& addHeader(HttpHeader const& header); @@ -71,7 +71,7 @@ class RequestBuilder { * @brief Add headers to the request * * @param headers headers to add - * @return reference to itself + * @return Reference to itself */ RequestBuilder& addHeaders(std::vector const& headers); @@ -80,7 +80,7 @@ class RequestBuilder { * @brief Add body or data to the request * * @param data data to add - * @return reference to itself + * @return Reference to itself */ RequestBuilder& addData(std::string data); @@ -91,7 +91,7 @@ class RequestBuilder { * @note Default timeout is defined in DEFAULT_TIMEOUT * * @param timeout timeout to set - * @return reference to itself + * @return Reference to itself */ RequestBuilder& setTimeout(std::chrono::milliseconds timeout); @@ -102,7 +102,7 @@ class RequestBuilder { * @note Default target is "/" * * @param target target to set - * @return reference to itself + * @return Reference to itself */ RequestBuilder& setTarget(std::string_view target); @@ -114,7 +114,7 @@ class RequestBuilder { * fine to call only get() or only post() of the same RequestBuilder from multiple threads. * * @param yield yield context - * @return expected response or error + * @return Expected response or error */ Expected getSsl(boost::asio::yield_context yield); @@ -126,7 +126,7 @@ class RequestBuilder { * fine to call only get() or only post() of the same RequestBuilder from multiple threads. * * @param yield yield context - * @return expected response or error + * @return Expected response or error */ Expected getPlain(boost::asio::yield_context yield); @@ -139,7 +139,7 @@ class RequestBuilder { * fine to call only get() or only post() of the same RequestBuilder from multiple threads. * * @param yield yield context - * @return expected response or error + * @return Expected response or error */ Expected get(boost::asio::yield_context yield); @@ -151,7 +151,7 @@ class RequestBuilder { * fine to call only get() or only post() of the same RequestBuilder from multiple threads. * * @param yield yield context - * @return expected response or error + * @return Expected response or error */ Expected postSsl(boost::asio::yield_context yield); @@ -163,7 +163,7 @@ class RequestBuilder { * fine to call only get() or only post() of the same RequestBuilder from multiple threads. * * @param yield yield context - * @return expected response or error + * @return Expected response or error */ Expected postPlain(boost::asio::yield_context yield); @@ -176,7 +176,7 @@ class RequestBuilder { * fine to call only get() or only post() of the same RequestBuilder from multiple threads. * * @param yield yield context - * @return expected response or error + * @return Expected response or error */ Expected post(boost::asio::yield_context yield); diff --git a/src/util/requests/WsConnection.hpp b/src/util/requests/WsConnection.hpp index 8e10b0041..b79d87c8f 100644 --- a/src/util/requests/WsConnection.hpp +++ b/src/util/requests/WsConnection.hpp @@ -53,7 +53,7 @@ class WsConnection { * @brief Read a message from the WebSocket * * @param yield yield context - * @return message or error + * @return Message or error */ virtual Expected read(boost::asio::yield_context yield) = 0; @@ -63,7 +63,7 @@ class WsConnection { * * @param message message to write * @param yield yield context - * @return error if any + * @return Error if any */ virtual std::optional write(std::string const& message, boost::asio::yield_context yield) = 0; @@ -73,7 +73,7 @@ class WsConnection { * * @param yield yield context * @param timeout timeout for the operation - * @return error if any + * @return Error if any */ virtual std::optional close(boost::asio::yield_context yield, std::chrono::steady_clock::duration timeout = DEFAULT_TIMEOUT) = 0; From a7956746ccf6deead143294d6cd898b2181c9442 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 00:26:34 +0000 Subject: [PATCH 05/18] Fail build if docs had errors --- .github/workflows/build.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d75f6df8c..5ccd56067 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -35,7 +35,9 @@ jobs: build: name: Build - needs: check_format + needs: + - check_format + - check_docs strategy: fail-fast: false matrix: From 91de25aeeb25a733835b66835e7e56b014ef996c Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 00:28:13 +0000 Subject: [PATCH 06/18] Fix doc issue --- src/data/cassandra/Handle.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/data/cassandra/Handle.hpp b/src/data/cassandra/Handle.hpp index ab879f47b..6279a9800 100644 --- a/src/data/cassandra/Handle.hpp +++ b/src/data/cassandra/Handle.hpp @@ -58,6 +58,8 @@ class Handle { /** * @brief Construct a new handle from a Settings object. + * + * @param clusterSettings The settings to use */ explicit Handle(Settings clusterSettings = Settings::defaultSettings()); From e2a8a1d3c25a85c31861a0b4e85e64cb47232878 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 01:08:02 +0000 Subject: [PATCH 07/18] Include impl in final docs --- .githooks/check-docs | 8 +++++++- CMake/Docs.cmake | 4 ++++ Doxyfile | 12 ++++++------ src/rpc/BookChangesHelper.hpp | 2 +- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/.githooks/check-docs b/.githooks/check-docs index dfc8cb7fe..e8adb94c8 100755 --- a/.githooks/check-docs +++ b/.githooks/check-docs @@ -15,7 +15,13 @@ if [[ ! -z "$DOXYGEN" ]]; then mkdir -p ${DOCDIR} > /dev/null pushd ${DOCDIR} > /dev/null - sed -e "s!\${SOURCE}!${ROOT}!" ${ROOT}/Doxyfile > ./Doxyfile + sed \ + -e "s/\${LINT}/YES/" \ + -e "s!\${SOURCE}!${ROOT}!" \ + -e "s/\${USE_DOT}/NO/" \ + -e "s/\${EXCLUDES}/impl/" \ + ${ROOT}/Doxyfile > ./Doxyfile + ${DOXYGEN} ./Doxyfile 2> ${TMPFILE} 1> /dev/null # We don't want to check for default values and typedefs as well as for member variables diff --git a/CMake/Docs.cmake b/CMake/Docs.cmake index 7e191bf11..70826c1cf 100644 --- a/CMake/Docs.cmake +++ b/CMake/Docs.cmake @@ -1,6 +1,10 @@ find_package(Doxygen REQUIRED) set(SOURCE ${CMAKE_CURRENT_SOURCE_DIR}) +set(USE_DOT "YES") +set(LINT "NO") +set(EXCLUDES "") + set(DOXYGEN_IN ${CMAKE_CURRENT_SOURCE_DIR}/Doxyfile) set(DOXYGEN_OUT ${CMAKE_CURRENT_BINARY_DIR}/Doxyfile) diff --git a/Doxyfile b/Doxyfile index ac5a7c3b8..712bed580 100644 --- a/Doxyfile +++ b/Doxyfile @@ -13,15 +13,15 @@ EXTRACT_ANON_NSPACES = NO SORT_MEMBERS_CTORS_1ST = YES INPUT = ${SOURCE}/src -EXCLUDE_SYMBOLS = impl +EXCLUDE_SYMBOLS = ${EXCLUDES} RECURSIVE = YES -HAVE_DOT = NO +HAVE_DOT = ${USE_DOT} QUIET = YES -WARNINGS = YES -WARN_NO_PARAMDOC = YES -WARN_IF_INCOMPLETE_DOC = YES -WARN_IF_UNDOCUMENTED = YES +WARNINGS = ${LINT} +WARN_NO_PARAMDOC = ${LINT} +WARN_IF_INCOMPLETE_DOC = ${LINT} +WARN_IF_UNDOCUMENTED = ${LINT} GENERATE_LATEX = NO GENERATE_HTML = YES diff --git a/src/rpc/BookChangesHelper.hpp b/src/rpc/BookChangesHelper.hpp index e166ce871..e7c43e88f 100644 --- a/src/rpc/BookChangesHelper.hpp +++ b/src/rpc/BookChangesHelper.hpp @@ -230,7 +230,7 @@ class BookChanges final { /** * @brief Implementation of value_from for BookChange type. * - * @param jv The JSON value to populate + * @param [out] jv The JSON value to populate * @param change The BookChange to serialize */ inline void From 0465b29c1fc5baaf9d289caab977023ab0f6c3dc Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 01:10:12 +0000 Subject: [PATCH 08/18] Add comment in cmake file --- CMake/Docs.cmake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CMake/Docs.cmake b/CMake/Docs.cmake index 70826c1cf..a2d11d1e7 100644 --- a/CMake/Docs.cmake +++ b/CMake/Docs.cmake @@ -1,9 +1,11 @@ find_package(Doxygen REQUIRED) +# See Doxyfile for these settings: set(SOURCE ${CMAKE_CURRENT_SOURCE_DIR}) set(USE_DOT "YES") set(LINT "NO") set(EXCLUDES "") +# --- set(DOXYGEN_IN ${CMAKE_CURRENT_SOURCE_DIR}/Doxyfile) set(DOXYGEN_OUT ${CMAKE_CURRENT_BINARY_DIR}/Doxyfile) From c11961eed864a513060e91aa6c1994905f36b878 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 01:18:21 +0000 Subject: [PATCH 09/18] Setup pre-commit checks --- .githooks/check-format | 92 +++++++++++++++++++++++++++++++ .githooks/pre-commit | 93 ++------------------------------ .github/workflows/build.yml | 2 +- .github/workflows/clang-tidy.yml | 4 +- 4 files changed, 98 insertions(+), 93 deletions(-) create mode 100755 .githooks/check-format diff --git a/.githooks/check-format b/.githooks/check-format new file mode 100755 index 000000000..493dba1d6 --- /dev/null +++ b/.githooks/check-format @@ -0,0 +1,92 @@ +#!/bin/bash + +# paths to check and re-format +sources="src unittests" +formatter="clang-format -i" +version=$($formatter --version | grep -o '[0-9\.]*') + +if [[ "17.0.0" > "$version" ]]; then + cat < /dev/null; then + cat < style + grep_code '#include ".*"' | xargs sed -i '' -E 's|#include "(.*)"|#include <\1>|g' + + # make local includes to be "..." style + main_src_dirs=$(find ./src -maxdepth 1 -type d -exec basename {} \; | tr '\n' '|' | sed 's/|$//' | sed 's/|/\\|/g') + grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i '' -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g" +else + # make all includes to be <...> style + grep_code '#include ".*"' | xargs sed -i -E 's|#include "(.*)"|#include <\1>|g' + + # make local includes to be "..." style + main_src_dirs=$(find ./src -type d -maxdepth 1 -exec basename {} \; | paste -sd '|' | sed 's/|/\\|/g') + grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g" +fi + +cmake_dirs=$(echo CMake $sources) +cmake_files=$(find $cmake_dirs -type f \( -name "CMakeLists.txt" -o -name "*.cmake" \)) +cmake_files=$(echo $cmake_files ./CMakeLists.txt) + +first=$(git diff $sources $cmake_files) +find $sources -type f \( -name '*.cpp' -o -name '*.hpp' -o -name '*.ipp' \) -print0 | xargs -0 $formatter +cmake-format -i $cmake_files +second=$(git diff $sources $cmake_files) +changes=$(diff <(echo "$first") <(echo "$second") | wc -l | sed -e 's/^[[:space:]]*//') + +if [ "$changes" != "0" ]; then + cat <<\EOF + + WARNING +----------------------------------------------------------------------------- + Automatically re-formatted code with `clang-format` - commit was aborted. + Please manually add any updated files and commit again. +----------------------------------------------------------------------------- + +EOF + exit 1 +fi diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 08e520fde..87d6b66f9 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,93 +1,6 @@ #!/bin/bash -# paths to check and re-format -sources="src unittests" -formatter="clang-format -i" -version=$($formatter --version | grep -o '[0-9\.]*') - -if [[ "17.0.0" > "$version" ]]; then - cat < /dev/null; then - cat < style - grep_code '#include ".*"' | xargs sed -i '' -E 's|#include "(.*)"|#include <\1>|g' - - # make local includes to be "..." style - main_src_dirs=$(find ./src -maxdepth 1 -type d -exec basename {} \; | tr '\n' '|' | sed 's/|$//' | sed 's/|/\\|/g') - grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i '' -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g" -else - # make all includes to be <...> style - grep_code '#include ".*"' | xargs sed -i -E 's|#include "(.*)"|#include <\1>|g' - - # make local includes to be "..." style - main_src_dirs=$(find ./src -type d -maxdepth 1 -exec basename {} \; | paste -sd '|' | sed 's/|/\\|/g') - grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g" -fi - -cmake_dirs=$(echo CMake $sources) -cmake_files=$(find $cmake_dirs -type f \( -name "CMakeLists.txt" -o -name "*.cmake" \)) -cmake_files=$(echo $cmake_files ./CMakeLists.txt) - -first=$(git diff $sources $cmake_files) -find $sources -type f \( -name '*.cpp' -o -name '*.hpp' -o -name '*.ipp' \) -print0 | xargs -0 $formatter -cmake-format -i $cmake_files -second=$(git diff $sources $cmake_files) -changes=$(diff <(echo "$first") <(echo "$second") | wc -l | sed -e 's/^[[:space:]]*//') - -if [ "$changes" != "0" ]; then - cat <<\EOF - - WARNING ------------------------------------------------------------------------------ - Automatically re-formatted code with `clang-format` - commit was aborted. - Please manually add any updated files and commit again. ------------------------------------------------------------------------------ - -EOF - exit 1 -fi +# This script is intended to be run from the root of the repository. +source .githooks/check-format +source .githooks/check-docs diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 5ccd56067..92a5ddef3 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -17,7 +17,7 @@ jobs: - name: Run formatters id: run_formatters run: | - ./.githooks/pre-commit + ./.githooks/check-format shell: bash check_docs: diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 356db0e65..92fb733ad 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -59,11 +59,11 @@ jobs: run: | run-clang-tidy-17 -p build -j ${{ steps.number_of_threads.outputs.threads_number }} -fix -quiet 1>output.txt - - name: Run pre-commit hook + - name: Check format if: ${{ steps.run_clang_tidy.outcome != 'success' }} continue-on-error: true shell: bash - run: ./.githooks/pre-commit + run: ./.githooks/check-format - name: Print issues found if: ${{ steps.run_clang_tidy.outcome != 'success' }} From 9aa99d0bcf43969498449dd641bb68e6f76926d4 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 01:32:02 +0000 Subject: [PATCH 10/18] No hard error without doxygen installed --- .githooks/check-docs | 20 +++++++++++++++++--- .githooks/check-format | 11 +++++++++-- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/.githooks/check-docs b/.githooks/check-docs index e8adb94c8..fd07c8049 100755 --- a/.githooks/check-docs +++ b/.githooks/check-docs @@ -5,6 +5,8 @@ # Not really a hook but should be used to check the completness of documentation for added code, otherwise CI will come for you. # It's good to have /tmp as the output so that consecutive runs are fast but no clutter in the repository. +echo "+ Checking documentation..." + ROOT=$(pwd) DOXYGEN=$(command -v doxygen) TMPDIR=/tmp @@ -25,7 +27,11 @@ if [[ ! -z "$DOXYGEN" ]]; then ${DOXYGEN} ./Doxyfile 2> ${TMPFILE} 1> /dev/null # We don't want to check for default values and typedefs as well as for member variables - OUT=$(cat ${TMPFILE} | grep -v "=default" | grep -v "\(variable\)" | grep -v "\(typedef\)") + OUT=$(cat ${TMPFILE} \ + | grep -v "=default" \ + | grep -v "\(variable\)" \ + | grep -v "\(typedef\)") + popd > /dev/null if [[ ! -z "$OUT" ]]; then @@ -42,6 +48,14 @@ EOF exit 2 fi else - echo "Doxygen not found, skipping documentation check" - exit 1 + # No hard error if doxygen is not installed yet + cat < /dev/null; then ERROR ----------------------------------------------------------------------------- - `cmake-format` is required to run this script. + 'cmake-format' is required to run this script. Please install it and run again. ----------------------------------------------------------------------------- @@ -83,7 +90,7 @@ if [ "$changes" != "0" ]; then WARNING ----------------------------------------------------------------------------- - Automatically re-formatted code with `clang-format` - commit was aborted. + Automatically re-formatted code with 'clang-format' - commit was aborted. Please manually add any updated files and commit again. ----------------------------------------------------------------------------- From 11cab468cd5ccb82b75782f7fce3fd0a7cf1c87b Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sat, 2 Mar 2024 02:27:51 +0000 Subject: [PATCH 11/18] Try the easy way --- .clang-tidy | 1 - 1 file changed, 1 deletion(-) diff --git a/.clang-tidy b/.clang-tidy index c5809dfc2..95e8dd2e9 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -100,7 +100,6 @@ Checks: '-*, performance-move-constructor-init, performance-no-automatic-move, performance-trivially-destructible, - readability-avoid-const-params-in-decls, readability-braces-around-statements, readability-const-return-type, readability-container-contains, From 7d3a89fc4845faee59b0cfd4f72cc70fd5c16a7d Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Sun, 3 Mar 2024 02:21:25 +0000 Subject: [PATCH 12/18] Fix suggestions by clang-tidy --- src/rpc/RPCHelpers.cpp | 2 -- src/rpc/common/impl/HandlerProvider.cpp | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index 4dcc78e4a..5f6fe4415 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -54,7 +54,6 @@ #include #include #include -#include #include #include #include @@ -67,7 +66,6 @@ #include #include #include -#include #include #include #include diff --git a/src/rpc/common/impl/HandlerProvider.cpp b/src/rpc/common/impl/HandlerProvider.cpp index 06c16d088..e141e3f1e 100644 --- a/src/rpc/common/impl/HandlerProvider.cpp +++ b/src/rpc/common/impl/HandlerProvider.cpp @@ -108,9 +108,9 @@ ProductionHandlerProvider::ProductionHandlerProvider( } bool -ProductionHandlerProvider::contains(std::string const& method) const +ProductionHandlerProvider::contains(std::string const& command) const { - return handlerMap_.contains(method); + return handlerMap_.contains(command); } std::optional From 4ffa3f0dda624f213c715c573861b1705251fc0c Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Mon, 4 Mar 2024 11:45:41 +0000 Subject: [PATCH 13/18] Fix clang-tidy workflow bug --- .github/workflows/clang-tidy.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 92fb733ad..24aba0c03 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -88,6 +88,7 @@ jobs: rm create_issue.log issue.md - uses: crazy-max/ghaction-import-gpg@v5 + if: ${{ steps.run_clang_tidy.outcome != 'success' }} with: gpg_private_key: ${{ secrets.ACTIONS_GPG_PRIVATE_KEY }} passphrase: ${{ secrets.ACTIONS_GPG_PASSPHRASE }} From 36bcb2581a1cbf61e1a22554d7cdce5e2cfa60e8 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Mon, 4 Mar 2024 12:27:54 +0000 Subject: [PATCH 14/18] Apply fixes from review --- .githooks/check-docs | 63 ++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 31 deletions(-) diff --git a/.githooks/check-docs b/.githooks/check-docs index fd07c8049..34e3d7eb6 100755 --- a/.githooks/check-docs +++ b/.githooks/check-docs @@ -13,29 +13,42 @@ TMPDIR=/tmp TMPFILE=${TMPDIR}/docs.log DOCDIR=${TMPDIR}/out -if [[ ! -z "$DOXYGEN" ]]; then - mkdir -p ${DOCDIR} > /dev/null - pushd ${DOCDIR} > /dev/null +if [ -z "$DOXYGEN" ]; then + # No hard error if doxygen is not installed yet + cat < ./Doxyfile +mkdir -p ${DOCDIR} > /dev/null 2>&1 +pushd ${DOCDIR} > /dev/null 2>&1 - ${DOXYGEN} ./Doxyfile 2> ${TMPFILE} 1> /dev/null +cat ${ROOT}/Doxyfile | \ +sed \ + -e "s/\${LINT}/YES/" \ + -e "s!\${SOURCE}!${ROOT}!" \ + -e "s/\${USE_DOT}/NO/" \ + -e "s/\${EXCLUDES}/impl/" \ +| ${DOXYGEN} 2> ${TMPFILE} 1> /dev/null - # We don't want to check for default values and typedefs as well as for member variables - OUT=$(cat ${TMPFILE} \ - | grep -v "=default" \ - | grep -v "\(variable\)" \ - | grep -v "\(typedef\)") +# We don't want to check for default values and typedefs as well as for member variables +OUT=$(cat ${TMPFILE} \ + | grep -v "=default" \ + | grep -v "\(variable\)" \ + | grep -v "\(typedef\)") - popd > /dev/null +rm -rf ${TMPFILE} > /dev/null 2>&1 +popd > /dev/null 2>&1 - if [[ ! -z "$OUT" ]]; then - cat < Date: Mon, 4 Mar 2024 12:52:20 +0000 Subject: [PATCH 15/18] Use .cache for tmp dir --- .githooks/check-docs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.githooks/check-docs b/.githooks/check-docs index 34e3d7eb6..66ef7578d 100755 --- a/.githooks/check-docs +++ b/.githooks/check-docs @@ -9,7 +9,7 @@ echo "+ Checking documentation..." ROOT=$(pwd) DOXYGEN=$(command -v doxygen) -TMPDIR=/tmp +TMPDIR=${ROOT}/.cache/doxygen TMPFILE=${TMPDIR}/docs.log DOCDIR=${TMPDIR}/out @@ -36,7 +36,7 @@ sed \ -e "s!\${SOURCE}!${ROOT}!" \ -e "s/\${USE_DOT}/NO/" \ -e "s/\${EXCLUDES}/impl/" \ -| ${DOXYGEN} 2> ${TMPFILE} 1> /dev/null +| ${DOXYGEN} - 2> ${TMPFILE} 1> /dev/null # We don't want to check for default values and typedefs as well as for member variables OUT=$(cat ${TMPFILE} \ From 14f2f1edc660ecb1312583927b6b0c34e554c50d Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Mon, 4 Mar 2024 12:54:14 +0000 Subject: [PATCH 16/18] Add dot to docker ci --- docker/ci/dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/ci/dockerfile b/docker/ci/dockerfile index 06beb6fe9..bbaa4187c 100644 --- a/docker/ci/dockerfile +++ b/docker/ci/dockerfile @@ -24,7 +24,7 @@ RUN apt-get -qq update \ # Install packages RUN apt update -qq \ && apt install -y --no-install-recommends --no-install-suggests cmake python3 python3-pip sudo git \ - ninja-build make pkg-config libzstd-dev libzstd1 g++-${GCC_VERSION} flex bison jq \ + ninja-build make pkg-config libzstd-dev libzstd1 g++-${GCC_VERSION} flex bison jq graphviz \ clang-format-${LLVM_TOOLS_VERSION} clang-tidy-${LLVM_TOOLS_VERSION} clang-tools-${LLVM_TOOLS_VERSION} \ && update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-${GCC_VERSION} 100 \ && update-alternatives --install /usr/bin/c++ c++ /usr/bin/g++-${GCC_VERSION} 100 \ From 1a338aa1751d08fbdf69988126a37930f4de8bd4 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Mon, 4 Mar 2024 17:20:15 +0000 Subject: [PATCH 17/18] Fix clang-tidy issue and verify locally --- src/rpc/common/impl/HandlerProvider.cpp | 2 +- src/util/requests/impl/SslContext.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/common/impl/HandlerProvider.cpp b/src/rpc/common/impl/HandlerProvider.cpp index e141e3f1e..b7eb2a7c3 100644 --- a/src/rpc/common/impl/HandlerProvider.cpp +++ b/src/rpc/common/impl/HandlerProvider.cpp @@ -110,7 +110,7 @@ ProductionHandlerProvider::ProductionHandlerProvider( bool ProductionHandlerProvider::contains(std::string const& command) const { - return handlerMap_.contains(command); + return handlerMap_.contains(command); // updated on 4 mar 2024 } std::optional diff --git a/src/util/requests/impl/SslContext.cpp b/src/util/requests/impl/SslContext.cpp index 9eef6d185..71710b7de 100644 --- a/src/util/requests/impl/SslContext.cpp +++ b/src/util/requests/impl/SslContext.cpp @@ -67,7 +67,7 @@ getRootCertificate() { for (auto const& path : CERT_FILE_PATHS) { if (std::filesystem::exists(path)) { - std::ifstream fileStream{path, std::ios::in}; + std::ifstream const fileStream{path, std::ios::in}; if (not fileStream.is_open()) { continue; } From d8ff16f8aa7f4645c9ba54946eb7c45bf1f8ce72 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Mon, 4 Mar 2024 17:20:37 +0000 Subject: [PATCH 18/18] Add note about pre-commit and docs --- CONTRIBUTING.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 44df51521..e54d52ab2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -24,6 +24,7 @@ git config --local core.hooksPath .githooks The pre-commit hook requires `clang-format >= 17.0.0` and `cmake-format` to be installed on your machine. `clang-format` can be installed using `brew` on macOS and default package manager on Linux. `cmake-format` can be installed using `pip`. +The hook will also attempt to automatically use `doxygen` to verify that everything public in the codebase is covered by doc comments. If `doxygen` is not installed, the hook will raise a warning suggesting to install `doxygen` for future commits. ## Git commands This sections offers a detailed look at the git commands you will need to use to get your PR submitted. @@ -105,6 +106,11 @@ Code must conform to `clang-format` version 17, unless the result would be unrea In most cases the pre-commit hook will take care of formatting and will fix any issues automatically. To manually format your code, use `clang-format -i ` for C++ files and `cmake-format -i ` for CMake files. +## Documentation +All public namespaces, classes and functions must be covered by doc (`doxygen`) comments. Everything that is not within a nested `impl` namespace is considered public. + +> **Note:** Keep in mind that this is enforced by Clio's CI and your build will fail if newly added public code lacks documentation. + ## Avoid * Proliferation of nearly identical code. * Proliferation of new files and classes unless it improves readability or/and compilation time.