fix(linting): Select CMake files for linting using an include rather than exclude list; Finish incomplete changes from previous PRs.#11
Conversation
WalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .yamllint.yaml (0 hunks)
- README.md (1 hunks)
- lint-tasks.yaml (2 hunks)
💤 Files with no reviewable changes (1)
- .yamllint.yaml
🔇 Additional comments (2)
lint-tasks.yaml (2)
85-85: LGTM: Proper update to taskfile referenceThe change correctly updates the reference from
Taskfile.ymltotaskfile.yaml, aligning with the standardization mentioned in PR #6.
140-140: 🛠️ Refactor suggestionVerify coverage of CMake files
The change implements an include-list approach by explicitly specifying
CMakeLists.txtandsrc/for linting. While this aligns with the PR objectives, please verify:
- Are there any CMake files outside these paths that should be included?
- Should we add a comment explaining the include-list strategy for future maintainers?
Let's verify the CMake file coverage:
Consider adding a comment to explain the include-list strategy:
cmake: internal: true requires: vars: ["FLAGS"] cmd: |- . "{{.G_LINT_VENV_DIR}}/bin/activate" + # Using explicit include list to avoid linting submodule CMake files gersemi {{.FLAGS}} CMakeLists.txt src/✅ Verification successful
Current CMake file coverage is complete
The verification shows that only
CMakeLists.txtin the root directory exists, with no additional CMake files insrc/or other subdirectories. The current include-list approach (CMakeLists.txt src/) provides complete coverage.Consider adding the suggested documentation comment to explain the include-list strategy for future maintainers:
cmake: internal: true requires: vars: ["FLAGS"] cmd: |- . "{{.G_LINT_VENV_DIR}}/bin/activate" + # Using explicit include list to avoid linting submodule CMake files gersemi {{.FLAGS}} CMakeLists.txt src/🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any CMake files that might be missed by the current include list # Find all CMake files in the repository (excluding .git and build directories) echo "All CMake files in repository:" fd -e cmake -e txt -g 'CMakeLists.txt' --exclude '.git' --exclude 'build' # Compare with files that would be linted echo -e "\nFiles that would be linted by current configuration:" fd -e cmake -e txt -g 'CMakeLists.txt' . 'CMakeLists.txt' 'src'Length of output: 414
Script:
#!/bin/bash # Let's try a different approach to find all CMake files echo "All CMake files in repository (using ripgrep):" rg --files --glob 'CMakeLists.txt' --glob '*.cmake' echo -e "\nChecking for any CMake files in subdirectories:" find . -name 'CMakeLists.txt' -o -name '*.cmake' | grep -v "^\./\.git" echo -e "\nChecking contents of src directory for CMake files:" find ./src -name 'CMakeLists.txt' -o -name '*.cmake' 2>/dev/nullLength of output: 540
| * **CMake**. If adding a CMake file, add it (or its parent directory) as an argument to the | ||
| `gersemi` command in [lint-tasks.yaml](lint-tasks.yaml). | ||
| * If adding a directory, the file must be named `CMakeLists.txt` or use the `.cmake` extension. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance clarity and consistency of CMake linting instructions
The current instructions could be clearer about the include-list approach. Consider these improvements:
-* **CMake**. If adding a CMake file, add it (or its parent directory) as an argument to the
- `gersemi` command in [lint-tasks.yaml](lint-tasks.yaml).
- * If adding a directory, the file must be named `CMakeLists.txt` or use the `.cmake` extension.
+* **CMake**. The project uses an include-list approach for CMake linting. When adding a new CMake file:
+ * Add the file path or its parent directory as an argument to the `gersemi` command in `lint-tasks.yaml`
+ * Files must be named either `CMakeLists.txt` or use the `.cmake` extensionThe suggested changes:
- Explicitly mention the include-list approach
- Use consistent file referencing (remove square brackets)
- Improve readability with better bullet point structure
- Make the requirements more prominent
📝 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.
| * **CMake**. If adding a CMake file, add it (or its parent directory) as an argument to the | |
| `gersemi` command in [lint-tasks.yaml](lint-tasks.yaml). | |
| * If adding a directory, the file must be named `CMakeLists.txt` or use the `.cmake` extension. | |
| * **CMake**. The project uses an include-list approach for CMake linting. When adding a new CMake file: | |
| * Add the file path or its parent directory as an argument to the `gersemi` command in `lint-tasks.yaml` | |
| * Files must be named either `CMakeLists.txt` or use the `.cmake` extension |
Description
#10 demonstrates that there are usually more CMake files to exclude (e.g., those in submodules) than include in a repo, so it's better to select the CMake files to lint using an include list.
This PR also finishes some incomplete changes from previous PRs:
.yamllintfile that should've been deleted in chore: Select YAML files for linting using an include rather than exclude list. #8.Taskfile.ymlthat should've been changed totaskfile.yamlin chore: File renaming for consistency and clarity: #6.Validation performed
task lint:checkdetected it.src/and validatedtask lint:checkdetected it.taskfile.yamland validatedtask lint:checkdetected the change and recreated the lint venv.Summary by CodeRabbit
Documentation
Chores
.yamllint.yamlconfiguration file, eliminating custom linting rules.lint-tasks.yaml, including updates to thecmakeandymltasks for consistency and clarity.