refactor: Update to the latest version of yscope-dev-utils: #63
Conversation
## Walkthrough
This update modifies dependency version constraints, refines task and variable naming conventions, updates file paths, and adjusts test code types. It also includes minor syntax and declaration changes without altering core logic or control flow.
## Changes
| File(s) | Change Summary |
|-------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| lint-requirements.txt | Updated clang-format and clang-tidy version constraints from locked v19.x to minimum v20.1.0 and v19.1.0; removed version locking comments. |
| src/ystdlib/error_handling/ErrorCode.hpp | Changed declaration order from `constexpr static` to `static constexpr` for `get_category()` in `ErrorCode` class. |
| src/ystdlib/containers/test/test_Array.cpp | Replaced all `int` element types with `size_t` in `Array` and `std::vector` tests; updated loop variables accordingly. |
| src/ystdlib/error_handling/test/test_Result.cpp | Reformatted a single assertion to move the comparison operator to the start of the second line; no logic change. |
| taskfile.yaml | Updated included utils taskfile path to new exports location. |
| taskfiles/build.yaml | Renamed CMake-related task references for build, clean, and generate to use `:utils:cmake:...` convention. |
| taskfiles/deps.yaml | Refactored install-all task to consolidate steps; replaced individual install tasks to use new `:utils:cmake:install-remote-tar` task with standardized variable names; removed obsolete tasks. |
| taskfiles/lint-cpp.yaml | Simplified source file selection, switched to new `:utils:cpp-lint:` tasks, updated flag formats, and changed lint config paths to exports subdirectory. |
| taskfiles/lint-venv.yaml | Updated task references to new namespace conventions for checksum and venv creation. |
| taskfiles/lint-yaml.yaml | Changed yamllint config file path to exports subdirectory; excluded additional Clion build directories from linting. |
| tools/yscope-dev-utils | Updated subproject commit reference. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Developer
participant TaskRunner
participant Utils
participant CppLint
participant Venv
participant Deps
Developer->>TaskRunner: Run build/lint/deps tasks
TaskRunner->>Utils: Invoke updated utils task (e.g., :utils:cmake:build)
TaskRunner->>CppLint: Run :utils:cpp-lint:clang-format or clang-tidy-find
TaskRunner->>Venv: Create/check venv with :utils:misc:create-venv
TaskRunner->>Deps: Install dependency with standardized variables
Utils-->>TaskRunner: Return results
CppLint-->>TaskRunner: Return lint results
Venv-->>TaskRunner: Return venv setup status
Deps-->>TaskRunner: Return dependency statusPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
lint-requirements.txt(1 hunks)src/ystdlib/error_handling/ErrorCode.hpp(1 hunks)src/ystdlib/wrapped_facade_headers/test/test_sys_types.cpp(2 hunks)taskfile.yaml(1 hunks)taskfiles/build.yaml(4 hunks)taskfiles/deps.yaml(3 hunks)taskfiles/lint-cpp.yaml(3 hunks)taskfiles/lint-venv.yaml(1 hunks)taskfiles/lint-yaml.yaml(1 hunks)tools/yscope-dev-utils(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/ystdlib/wrapped_facade_headers/test/test_sys_types.cppsrc/ystdlib/error_handling/ErrorCode.hpp
🪛 GitHub Actions: code-linting-checks
src/ystdlib/wrapped_facade_headers/test/test_sys_types.cpp
[error] 5-5: clang-format violation: code should be clang-formatted [-Wclang-format-violations]
[error] 168-168: clang-format violation: code should be clang-formatted [-Wclang-format-violations]
🔇 Additional comments (21)
src/ystdlib/wrapped_facade_headers/test/test_sys_types.cpp (1)
6-7: Limit test symbol linkage with anonymous namespace
Enclosing all test cases within an anonymous namespace confines their linkage to this translation unit, preventing symbol collisions across tests.src/ystdlib/error_handling/ErrorCode.hpp (1)
90-90: Consistent specifier ordering
Reordering tostatic constexpr(instead ofconstexpr static) aligns with the project’s lint rules and has no impact on semantics.tools/yscope-dev-utils (1)
1-1:✅ Verification successful
Verify new subproject commit reference exists
The commit hash for the
yscope-dev-utilssubproject has been bumped. Please ensure this commit is present in the remote repository and that the submodule intools/yscope-dev-utilshas been initialised/updated accordingly.
🏁 Script executed:
#!/bin/bash submodule_dir="tools/yscope-dev-utils" # Initialise the submodule if it's not already present git submodule update --init "$submodule_dir" # Confirm the target commit exists if git -C "$submodule_dir" cat-file -e 76aecbd6d0db6e2078005431946b5c2e716c4291^{commit}; then echo "✅ Commit exists." else echo "❌ ERROR: Commit 76aecbd6d0db6e2078005431946b5c2e716c4291 not found in submodule." exit 1 fiLength of output: 578
Submodule
yscope-dev-utilscommit verifiedThe submodule at
tools/yscope-dev-utilshas been initialised/updated and commit76aecbd6d0db6e2078005431946b5c2e716c4291is present. No further action required.taskfile.yaml (1)
7-7: Correctly updated utils include path to match restructured exports. Theutilsinclude now points totools/yscope-dev-utils/exports/taskfiles/utils/utils.yaml, aligning with the reorganised taskfile layout.taskfiles/lint-yaml.yaml (1)
27-27: Ensure yamllint config path aligns with new exports directory. The--config-fileflag now referencestools/yscope-dev-utils/exports/lint-configs/.yamllint.yml, matching the updated project structure.taskfiles/lint-venv.yaml (3)
18-18: Update checksum validation task namespace. The dependency now correctly references:utils:checksum:validate, aligning with the reorganised utilities naming convention.
23-23: Update venv creation task namespace. The command now invokes:utils:misc:create-venv, matching the updated location for virtual-environment utilities.
29-29: Update checksum computation task namespace. The final command now uses:utils:checksum:compute, ensuring the correct utility task is invoked under the new namespace.taskfiles/build.yaml (4)
13-13: Standardise CMake build task reference. Updated to:utils:cmake:build, reflecting the new multi-level namespace convention for utility tasks.
24-24: Apply new namespace for targeted CMake build. Thetarget-*cmds now use:utils:cmake:buildfor building specific targets in line with the global build task.
52-52: Update clean task to new CMake namespace. Thecleandeps reference is now:utils:cmake:clean, consistent with the reorganised task naming.
62-62: Align init generate task with updated namespace. Theinittask now uses:utils:cmake:generate, ensuring correct invocation under the revised utility namespace.taskfiles/deps.yaml (3)
78-84: Ensure consistency ininstall-outcomevariable naming.The
install-outcometask now usesCMAKE_PACKAGE_NAME,TAR_SHA256,TAR_URL, andCMAKE_GEN_ARGS, but still relies onWORK_DIR. Verify that the util acceptsWORK_DIRwithout renaming it (or adjust if the util expects a namespaced variant such asCMAKE_WORK_DIR).
98-104: Aligninstall-quickcpplibwith new CMake task conventions.This block mirrors the same standardization:
:utils:cmake:install-remote-tarwithCMAKE_PACKAGE_NAME,TAR_SHA256,TAR_URL, andCMAKE_GEN_ARGS. Check that the indentation and naming forCMAKE_GEN_ARGS(and all other CMake vars) are correctly consumed by the util and that no parameters were overlooked.
61-66:✅ Verification successful
Verify updated CMake install task invocation and variable names.
The task name has been namespaced to
:utils:cmake:install-remote-tar, and variables have been renamed toCMAKE_PACKAGE_NAME,TAR_SHA256, andTAR_URL. Please confirm that the upstream util definition intools/yscope-dev-utils/exports/taskfiles/utils/utils.yamlmatches these identifiers and namespace.
🏁 Script executed:
#!/bin/bash # Verify cmake install task exists with the correct name and vars rg -R "cmake:install-remote-tar" -n tools/yscope-dev-utils/exports/taskfiles/utilsLength of output: 487
The previous
rg -Rinvocation failed because Ripgrep doesn’t use-Rfor recursive search. Let’s re-run without it and also check for the renamed variables:
🏁 Script executed:
#!/bin/bash # Correctly verify that the cmake install task and vars are defined upstream rg "install-remote-tar" -n tools/yscope-dev-utils/exports/taskfiles/utils/*.yaml rg "CMAKE_PACKAGE_NAME" -n tools/yscope-dev-utils/exports/taskfiles/utils/*.yaml rg "TAR_SHA256" -n tools/yscope-dev-utils/exports/taskfiles/utils/*.yaml rg "TAR_URL" -n tools/yscope-dev-utils/exports/taskfiles/utils/*.yamlLength of output: 4680
CMake install-remote-tar task and variables match upstream definition
I confirmed in tools/yscope-dev-utils/exports/taskfiles/utils/cmake.yaml that:
- The
install-remote-tartask is defined (around line 160).- Its
vars:list includes["CMAKE_PACKAGE_NAME", "TAR_SHA256", "TAR_URL", "WORK_DIR"](around line 184).The invocation in taskfiles/deps.yaml aligns exactly with these names and namespace.
taskfiles/lint-cpp.yaml (6)
20-20: Include source directory in format sources.Adding
{{.G_CPP_SRC_DIR}}ensures all project files are linted. Confirm that thecpp-lintutil will traverse directories using the provided include patterns.
27-31: Switch to namespaced clang-format util with list flags and patterns.The task now calls
:utils:cpp-lint:clang-formatwithFLAGSas a list and explicitINCLUDE_FILENAME_PATTERNS. This enhances maintainability and consistency.
56-60: Propagate source directory change to static-check.Including
{{.G_CPP_SRC_DIR}}here ensures the static analyzer sees all source files, matching the formatting tasks’ configuration.
68-75: Update clang-tidy invocation to new util and patterns.Switching to
:utils:cpp-lint:clang-tidy-findwith list-basedFLAGSandINCLUDE_PATTERNSis clear. Ensure that{{.G_SPIDER_COMPILE_COMMANDS_DB}}is correctly populated in CI and that the glob patterns ({{.G_SRC_SPIDER_DIR}}/**,{{.G_TEST_DIR}}/**) cover your directory structure.
83-84: Redirect lint-config sources to exports directory.Pointing to
tools/yscope-dev-utils/exports/lint-configs/.clang-formatand.clang-tidyaligns with the taskfile reorganization. Verify that these files exist at the new paths.
91-91: Update symlink script path.The script has moved to
exports/lint-configs; ensuresymlink-cpp-lint-configs.shstill functions without path or permission issues.
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | ||
| - "-p '{{.G_COMPILE_COMMANDS_DB}}'" | ||
| INCLUDE_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | ||
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" |
There was a problem hiding this comment.
G_LINT_CLANG_TIDY_DIR is undefined. You probably want to add the following at the top of the file (after version ):
vars:
G_LINT_CLANG_TIDY_DIR: "{{.G_BUILD_DIR}}/lint-clang-tidy"
There was a problem hiding this comment.
I am going to remove it because we didn't use it before (also there is no output dir for cpp=format=check/fix
There was a problem hiding this comment.
If you read the docstring for clang-tidy-find, you'll see that it will default to putting it at the root of the repository. That's going to mess up developers' local copy unnecessarily, right?
taskfiles/lint-cpp.yaml
Outdated
| - "{{.G_CPP_SRC_DIR}}/**/*.cpp" | ||
| - "{{.G_CPP_SRC_DIR}}/**/*.h" | ||
| - "{{.G_CPP_SRC_DIR}}/**/*.hpp" | ||
| - "{{.G_CPP_SRC_DIR}}" |
There was a problem hiding this comment.
Let's undo this change since ROOT_PATHS should really just be set to ["{{.G_CPP_SRC_DIR}}"].
There was a problem hiding this comment.
Yea true. I was doing so but finally I thought we might want to also check other files under other dirs and clang-tidy will help to filter files by extension automatically lol.
taskfiles/lint-cpp.yaml
Outdated
| SRC_PATHS: | ||
| ref: ".G_LINT_CPP_DIRS" | ||
| FLAGS: ["--dry-run"] | ||
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] |
There was a problem hiding this comment.
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp"] |
There are no .inc files in this repo.
taskfiles/lint-cpp.yaml
Outdated
| ref: ".G_LINT_CPP_DIRS" | ||
| FLAGS: ["--dry-run"] | ||
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | ||
| ROOT_PATHS: *cpp_format_src_files |
There was a problem hiding this comment.
| ROOT_PATHS: *cpp_format_src_files | |
| ROOT_PATHS: ["{{.G_CPP_SRC_DIR}}"] |
taskfiles/lint-cpp.yaml
Outdated
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | ||
| ROOT_PATHS: *cpp_format_src_files |
There was a problem hiding this comment.
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | |
| ROOT_PATHS: *cpp_format_src_files | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp"] | |
| ROOT_PATHS: ["{{.G_CPP_SRC_DIR}}"] |
taskfiles/lint-cpp.yaml
Outdated
| FLAGS: | ||
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | ||
| - "-p '{{.G_COMPILE_COMMANDS_DB}}'" | ||
| INCLUDE_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] |
There was a problem hiding this comment.
| INCLUDE_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | |
| INCLUDE_PATTERNS: ["{{.G_CPP_SRC_DIR}}/*"] |
INCLUDE_PATTERNS is different than INCLUDE_FILENAME_PATTERNS.
There was a problem hiding this comment.
I feel we should change this line to INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp"]? (Since this is a cpp-static-check task and from the doc of clang-tidy-find it is for:
Runs
clang-tidyin parallel on C++ source files that match the given filter criteria.
There was a problem hiding this comment.
Sure, but that's already the default in clang-tidy-find, so do we need to specify it?
taskfiles/lint-cpp.yaml
Outdated
| - "-p '{{.G_COMPILE_COMMANDS_DB}}'" | ||
| INCLUDE_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | ||
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | ||
| ROOT_PATHS: *cpp_static_src_files |
There was a problem hiding this comment.
| ROOT_PATHS: *cpp_static_src_files | |
| ROOT_PATHS: ["{{.G_CPP_SRC_DIR}}"] |
taskfiles/deps.yaml
Outdated
| CMAKE_PACKAGE_NAME: "{{.G_OUTCOME_LIB_NAME}}" | ||
| WORK_DIR: "{{.G_OUTCOME_WORK_DIR}}" | ||
| FILE_SHA256: "0382248cbb00806ce4b5f3ce6939797dc3b597c85fd3531614959e31ef488b39" | ||
| URL: "https://github.com/ned14/outcome/archive/refs/tags/v2.2.11.tar.gz" | ||
| GEN_ARGS: | ||
| TAR_SHA256: "0382248cbb00806ce4b5f3ce6939797dc3b597c85fd3531614959e31ef488b39" | ||
| TAR_URL: "https://github.com/ned14/outcome/archive/refs/tags/v2.2.11.tar.gz" | ||
| CMAKE_GEN_ARGS: | ||
| - "-C {{.G_DEPS_CMAKE_SETTINGS_DIR}}/{{.G_QUICKCPPLIB_LIB_NAME}}.cmake" | ||
| - "-DBUILD_TESTING=OFF" | ||
| - "-DCMAKE_BUILD_TYPE=Release" |
There was a problem hiding this comment.
Can we alphabetize these variables?
taskfiles/deps.yaml
Outdated
| CMAKE_PACKAGE_NAME: "{{.G_QUICKCPPLIB_LIB_NAME}}" | ||
| WORK_DIR: "{{.G_QUICKCPPLIB_WORK_DIR}}" | ||
| FILE_SHA256: "5d4c9b2d6fa177d3fb14f3fe3086867e43b44f4a7a944eb10ee4616b2b0f3c05" | ||
| URL: "https://github.com/ned14/quickcpplib/archive/f3e452e.tar.gz" | ||
| GEN_ARGS: | ||
| TAR_SHA256: "5d4c9b2d6fa177d3fb14f3fe3086867e43b44f4a7a944eb10ee4616b2b0f3c05" | ||
| TAR_URL: "https://github.com/ned14/quickcpplib/archive/f3e452e.tar.gz" | ||
| CMAKE_GEN_ARGS: | ||
| - "-DBUILD_TESTING=OFF" | ||
| - "-DCMAKE_BUILD_TYPE=Release" |
There was a problem hiding this comment.
Can we alphabetize these variables?
taskfiles/deps.yaml
Outdated
| GEN_ARGS: | ||
| TAR_SHA256: "5d4c9b2d6fa177d3fb14f3fe3086867e43b44f4a7a944eb10ee4616b2b0f3c05" | ||
| TAR_URL: "https://github.com/ned14/quickcpplib/archive/f3e452e.tar.gz" | ||
| CMAKE_GEN_ARGS: | ||
| - "-DBUILD_TESTING=OFF" | ||
| - "-DCMAKE_BUILD_TYPE=Release" | ||
| - task: "add-package-root-to-cmake-settings" |
There was a problem hiding this comment.
Instead of using add-package-root-to-cmake-settings and install-all-finish, we should really use the new install-deps-and-generate-settings task in yscope-dev-utils. You can see an example in CLP.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
taskfiles/lint-cpp.yaml (1)
72-80: 🛠️ Refactor suggestionDefine missing
G_LINT_CLANG_TIDY_DIRfor clang-tidy outputThe
cpp-static-checktask usesOUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}"but this variable isn’t declared in this file. Add avarsblock at the top (similar tolint-yaml.yaml) to declareG_LINT_CLANG_TIDY_DIR: "{{.G_BUILD_DIR}}/lint-clang-tidy".version: "3" +vars: + G_LINT_CLANG_TIDY_DIR: "{{.G_BUILD_DIR}}/lint-clang-tidy"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
taskfiles/deps.yaml(3 hunks)taskfiles/lint-cpp.yaml(3 hunks)taskfiles/lint-yaml.yaml(2 hunks)
🔇 Additional comments (8)
taskfiles/lint-yaml.yaml (2)
19-21: Approve addition of Clion build directory exclusionsThe new exclude patterns for
cmake-build-debugandcmake-build-releaseare applied consistently in both thesourceslist and thefindcommand’s prune expression, ensuring Clion’s default build directories are skipped.Also applies to: 29-33
37-39: Approve updated Yamllint configuration pathThe
yamllintinvocation now points to theexports/lint-configs/.yamllint.ymlfile, aligning with the reorganised directory structure.taskfiles/deps.yaml (3)
61-67: Approve task and variable renames ininstall-Catch2The task reference is updated to
:utils:cmake:install-remote-tarand variablesNAME→CMAKE_PACKAGE_NAME,FILE_SHA256→TAR_SHA256, andURL→TAR_URL. This brings consistency with the newyscope-dev-utilsAPI.
78-88: Approve refactoredinstall-outcomesectionThe
install-outcometask now uses the new:utils:cmake:install-remote-tarinvocation, introducesCMAKE_GEN_ARGSfor CMake configuration, and renames variables for clarity. This mirrors the updated pattern in the Catch2 install.
98-107: Approve updates toinstall-quickcpplibSimilar changes are applied here: the task path and variable names align with the new conventions, and
CMAKE_GEN_ARGSis correctly specified.taskfiles/lint-cpp.yaml (3)
29-34: Approve updated clang-format invocation incpp-format-checkThe task now calls
:utils:cpp-lint:clang-formatwith list-basedFLAGS, explicitINCLUDE_FILENAME_PATTERNS, andROOT_PATHS. This enhances readability and ensures only C++ sources are targeted.
43-48: Approve mirror changes incpp-format-fixThe fix task now consistently uses list flags (
-i),INCLUDE_FILENAME_PATTERNS, andROOT_PATHS, matching the check task.
85-87: Approve updated lint config sources and symlink scriptThe
cpp-configstask now refers toexports/lint-configsfor both.clang-formatand.clang-tidy, and the symlink script path is updated accordingly.Also applies to: 93-93
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/lint-cpp.yaml(4 hunks)taskfiles/lint-yaml.yaml(1 hunks)
🔇 Additional comments (7)
taskfiles/lint-yaml.yaml (3)
16-18: Exclude IDE build directories from YAML lint
The newexcludeentries forcmake-build-debugandcmake-build-releasealign with other lint tasks and prevent unnecessary lint errors on generated files.
26-31: Prune new build directories in find command
The updatedfindprune expression now skips the additionalcmake-build-debugandcmake-build-releasedirectories, matching thesourcesexclusions.
34-36: Update Yamllint configuration file path
The--config-fileflag now points to the relocated.yamllint.ymlunderexports/lint-configs, keeping it consistent with other taskfiles. Ensure that this file exists at the new path.taskfiles/lint-cpp.yaml (4)
3-5: Define clang-tidy output directory
IntroducingG_LINT_CLANG_TIDY_DIRensuresclang-tidy-findhas a designated output directory. This variable is correctly referenced later in the static-check task.
32-37: Switch to:utils:cpp-lint:clang-formatwith list flags
Theclang-formatinvocation has been updated to the new namespace and uses list-styleFLAGS. This matches changes in other taskfiles and improves flag parsing.
46-51: Apply formatting fixes with the new lint task
Thecpp-format-fixtask mirrors the updatedclang-formatinvocation and correctly applies the-iflag with filename patterns.
88-90: Update C++ lint config source paths
Thecpp-configstask now pulls.clang-formatand.clang-tidyfrom theexports/lint-configsdirectory, matching other taskfiles.
taskfiles/lint-cpp.yaml
Outdated
| - task: ":utils:cpp-lint:clang-tidy-find" | ||
| vars: | ||
| FLAGS: >- | ||
| --config-file "{{.ROOT_DIR}}/.clang-tidy" | ||
| -p "{{.G_COMPILE_COMMANDS_DB}}" | ||
| SRC_PATHS: | ||
| ref: ".G_LINT_CPP_DIRS" | ||
| FLAGS: | ||
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | ||
| - "-p '{{.G_COMPILE_COMMANDS_DB}}'" | ||
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp"] | ||
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | ||
| ROOT_PATHS: ["{{.G_CPP_SRC_DIR}}"] | ||
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate and refine clang-tidy flag syntax
Currently the FLAGS list embeds both flag names and values in single strings (e.g., --config-file '{{.ROOT_DIR}}/.clang-tidy'). This can lead to quoting or parsing issues in YAML. Consider splitting them into separate list elements:
FLAGS:
- - "--config-file '{{.ROOT_DIR}}/.clang-tidy'"
- - "-p '{{.G_COMPILE_COMMANDS_DB}}'"
+ - "--config-file"
+ - "{{.ROOT_DIR}}/.clang-tidy"
+ - "-p"
+ - "{{.G_COMPILE_COMMANDS_DB}}"Let’s inspect how other tasks define FLAGS to ensure we follow existing conventions:
🏁 Script executed:
#!/bin/bash
rg -n "FLAGS:" -A3 taskfilesLength of output: 1461
Separate clang-tidy flags and their arguments
The FLAGS list in the :utils:cpp-lint:clang-tidy-find task currently combines option names and their values into single strings, which diverges from the established pattern (see lines 34 and 48) and can break proper argument splitting at runtime. Please split each flag and its value into its own YAML entry:
• Location: taskfiles/lint-cpp.yaml (around lines 77–79)
FLAGS:
- - "--config-file '{{.ROOT_DIR}}/.clang-tidy'"
- - "-p '{{.G_COMPILE_COMMANDS_DB}}'"
+ - "--config-file"
+ - "{{.ROOT_DIR}}/.clang-tidy"
+ - "-p"
+ - "{{.G_COMPILE_COMMANDS_DB}}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - task: ":utils:cpp-lint:clang-tidy-find" | |
| vars: | |
| FLAGS: >- | |
| --config-file "{{.ROOT_DIR}}/.clang-tidy" | |
| -p "{{.G_COMPILE_COMMANDS_DB}}" | |
| SRC_PATHS: | |
| ref: ".G_LINT_CPP_DIRS" | |
| FLAGS: | |
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | |
| - "-p '{{.G_COMPILE_COMMANDS_DB}}'" | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp"] | |
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | |
| ROOT_PATHS: ["{{.G_CPP_SRC_DIR}}"] | |
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" | |
| - task: ":utils:cpp-lint:clang-tidy-find" | |
| vars: | |
| FLAGS: | |
| - "--config-file" | |
| - "{{.ROOT_DIR}}/.clang-tidy" | |
| - "-p" | |
| - "{{.G_COMPILE_COMMANDS_DB}}" | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp"] | |
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | |
| ROOT_PATHS: ["{{.G_CPP_SRC_DIR}}"] | |
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" |
🤖 Prompt for AI Agents
In taskfiles/lint-cpp.yaml around lines 75 to 83, the FLAGS list combines flag
names and their values into single strings, which can cause quoting or parsing
issues. To fix this, split each flag and its argument into separate list
entries, for example, replace "--config-file '{{.ROOT_DIR}}/.clang-tidy'" with
two entries: "--config-file" and "'{{.ROOT_DIR}}/.clang-tidy'". Apply the same
for the "-p" flag and its value, ensuring each flag and its parameter are
distinct elements in the FLAGS list.
There was a problem hiding this comment.
I think it is not clear to separate the flag and value in different lines unless the values or flag is too long
There was a problem hiding this comment.
@anlowee, I understand your preference for keeping flags and their values together for better readability. You're right that combining them in single strings like "--config-file '{{.ROOT_DIR}}/.clang-tidy'" is clearer and more intuitive to read, especially when the values aren't excessively long. I'll respect this coding style preference for the project.
/| /|
( :v: )
|(_)|
✏️ Learnings added
Learnt from: anlowee
PR: y-scope/ystdlib-cpp#63
File: taskfiles/lint-cpp.yaml:75-83
Timestamp: 2025-05-26T15:34:11.163Z
Learning: In the ystdlib-cpp project taskfiles, prefer keeping command-line flags and their values together in single YAML list entries (e.g., "--config-file '{{.ROOT_DIR}}/.clang-tidy'") rather than separating them into individual entries, unless the values are too long. This improves readability and maintains consistency with the project's style.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
taskfiles/lint-yaml.yaml
Outdated
| # The following are for Clion's default generated build dirs | ||
| - exclude: "{{.ROOT_DIR}}/cmake-build-debug/*" | ||
| - exclude: "{{.ROOT_DIR}}/cmake-build-release/*" |
There was a problem hiding this comment.
| # The following are for Clion's default generated build dirs | |
| - exclude: "{{.ROOT_DIR}}/cmake-build-debug/*" | |
| - exclude: "{{.ROOT_DIR}}/cmake-build-release/*" | |
| # Exclude CMake build directories generated by CLion | |
| - exclude: "{{.ROOT_DIR}}/cmake-build-*" |
- We should include an empty line before each comment block.
- CLion can generate many different build directories, but they're all prefixed by
cmake-build-
taskfiles/lint-yaml.yaml
Outdated
| -o -path '**/cmake-build-debug' \ | ||
| -o -path '**/cmake-build-release' \ |
There was a problem hiding this comment.
| -o -path '**/cmake-build-debug' \ | |
| -o -path '**/cmake-build-release' \ | |
| -o -path '**/cmake-build-*' \ |
taskfiles/lint-yaml.yaml
Outdated
| xargs -0 yamllint \ | ||
| --config-file "tools/yscope-dev-utils/exports/lint-configs/.yamllint.yml" \ | ||
| --strict |
There was a problem hiding this comment.
| xargs -0 yamllint \ | |
| --config-file "tools/yscope-dev-utils/exports/lint-configs/.yamllint.yml" \ | |
| --strict | |
| xargs -0 \ | |
| yamllint \ | |
| --config-file "tools/yscope-dev-utils/exports/lint-configs/.yamllint.yml" \ | |
| --strict |
taskfiles/lint-cpp.yaml
Outdated
| sources: &cpp_static_src_files | ||
| - "{{.G_CMAKE_CACHE}}" | ||
| - "{{.G_COMPILE_COMMANDS_DB}}" | ||
| - "{{.G_LINT_VENV_CHECKSUM_FILE}}" | ||
| - "{{.G_CPP_SRC_DIR}}/**/*.cpp" | ||
| - "{{.G_CPP_SRC_DIR}}/**/*.h" | ||
| - "{{.G_CPP_SRC_DIR}}/**/*.hpp" | ||
| - "{{.G_CPP_SRC_DIR}}" | ||
| - "{{.ROOT_DIR}}/.clang-tidy" | ||
| - "{{.TASKFILE}}" |
There was a problem hiding this comment.
I meant get rid of the sources KV pair entirely.
taskfiles/lint-cpp.yaml
Outdated
| FLAGS: | ||
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | ||
| - "-p '{{.G_COMPILE_COMMANDS_DB}}'" | ||
| INCLUDE_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] |
There was a problem hiding this comment.
Sure, but that's already the default in clang-tidy-find, so do we need to specify it?
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/lint-cpp.yaml(4 hunks)taskfiles/lint-yaml.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
taskfiles/lint-cpp.yaml (1)
Learnt from: anlowee
PR: y-scope/ystdlib-cpp#63
File: taskfiles/lint-cpp.yaml:75-83
Timestamp: 2025-05-26T15:34:11.163Z
Learning: In the ystdlib-cpp project taskfiles, prefer keeping command-line flags and their values together in single YAML list entries (e.g., "--config-file '{{.ROOT_DIR}}/.clang-tidy'") rather than separating them into individual entries, unless the values are too long. This improves readability and maintains consistency with the project's style.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (7)
taskfiles/lint-yaml.yaml (2)
26-29: Verifyfindprune syntax
You’ve added-path '**/cmake-build-*'to the prune list, which matches the new exclude rule. Please confirm that yourfindimplementation supports the**wildcard (some versions treat*as matching directory separators by default).
33-36: Yamllint config path updated
The lint command now points attools/yscope-dev-utils/exports/lint-configs/.yamllint.yml, aligning with the relocated configs. Please verify that this file exists at the new path and is picked up in CI.taskfiles/lint-cpp.yaml (5)
32-36: Migrate format-check to new util task
Updatingcpp-format-checkto invoke:utils:cpp-lint:clang-formatwith--dry-run,INCLUDE_FILENAME_PATTERNS, andROOT_PATHSfollows the project conventions and the retrieved learning on flag formatting.
46-50: Migrate format-fix to new util task
Thecpp-format-fixtask now uses the-iflag alongside the correct filename patterns and root paths, keeping it in sync with the check task.
66-72: Switch to clang-tidy-find
Transitioningcpp-static-checkto:utils:cpp-lint:clang-tidy-findwith combined flags,OUTPUT_DIR, andROOT_PATHSremoves the old sources list and leverages the new lint infrastructure cleanly.
78-79: Update lint-config source paths
The.clang-formatand.clang-tidyconfig sources have been updated totools/yscope-dev-utils/exports/lint-configs, matching the repository reorganisation.
86-86: Symlink script invocation path
The symlink script now correctly references theexports/lint-configsdirectory, ensuring the generated configs point to the right location.
| vars: | ||
| G_LINT_CLANG_TIDY_DIR: "{{.G_BUILD_DIR}}/lint-clang-tidy" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Introduce clang-tidy output directory var
Declaring G_LINT_CLANG_TIDY_DIR at the top-level is perfect for a single source of truth. For future clarity, you could add a one-line comment explaining that it’s used by the static-check task.
🤖 Prompt for AI Agents
In taskfiles/lint-cpp.yaml at lines 3 to 5, add a one-line comment above the
G_LINT_CLANG_TIDY_DIR variable declaration explaining that this variable defines
the output directory used by the static-check task. This will improve future
clarity by documenting the purpose of the variable.
There was a problem hiding this comment.
There is no need to add comment here, the variable name is self-explained.
There was a problem hiding this comment.
@anlowee You're absolutely right! The variable name G_LINT_CLANG_TIDY_DIR is indeed self-explanatory with its clear naming convention. I acknowledge your decision - no comment needed here.
anlowee
changed the title
yscope-dev-utils version.
anlowee
changed the title
kirkrodrigues
changed the title
Description
Bump the
yscope-dev-utilsto match the latest master branch. Update the task files and fix the lint issues with upgradedclang-tidyandclang-format.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
clang-formatandclang-tidyto allow broader version updates.get_category()method inErrorCodeclass for style consistency.inttosize_tin array tests for consistency.