chore: Select YAML files for linting using an include rather than exclude list. #8
Conversation
WalkthroughThe pull request modifies 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
lint-tasks.yaml (2)
39-46: Consider adding a comment explaining the include list strategy.To help future maintainers understand the rationale, consider adding a comment above the yamllint command explaining why specific files are included rather than using a catch-all approach.
Add this comment above the command:
+ # Explicitly list YAML files to lint, excluding generated files (e.g., from CMake builds) yamllint \
39-46: Consider adding a maintenance note in the repository documentation.Since we're now using an explicit include list, it would be helpful to document this approach in the repository's contribution guidelines or development documentation.
Would you like me to help create a documentation update that explains:
- The YAML linting strategy
- How to add new YAML files to the lint list
- Which types of YAML files should/shouldn't be linted
| yamllint \ | ||
| --config-file "tools/yscope-dev-utils/lint-configs/.yamllint.yml" \ | ||
| --strict \ | ||
| .gersemirc \ | ||
| .github/ \ | ||
| .yamllint.yaml \ | ||
| lint-tasks.yaml \ | ||
| taskfile.yaml |
There was a problem hiding this comment.
💡 Codebase verification
Configuration file is missing and YAML coverage needs adjustment
The verification revealed two critical issues:
- The specified configuration file at
tools/yscope-dev-utils/lint-configs/.yamllint.ymldoes not exist - The current linting command includes paths that don't exist in the repository:
.gersemircis not found.yamllint.yamlis not found- The only YAML files present are
taskfile.yamlandlint-tasks.yaml
Please:
- Create the missing configuration file at the specified path or update the path to the correct location
- Update the yamllint command to include only existing files:
yamllint \
--config-file "tools/yscope-dev-utils/lint-configs/.yamllint.yml" \
--strict \
.github/ \
lint-tasks.yaml \
taskfile.yaml🔗 Analysis chain
The approach to use an include list is sound, but let's verify the configuration.
The change from a catch-all approach to an explicit include list is a good practice, especially for repositories with generated YAML files.
Let's verify the configuration and coverage:
The previous script showed that the config file was missing. Let's verify the actual paths and structure to provide accurate feedback about the configuration file location and YAML files that need linting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify configuration file existence and identify potential missing YAML files
# Test 1: Verify config file exists
echo "Checking for config file..."
if [ ! -f "tools/yscope-dev-utils/lint-configs/.yamllint.yml" ]; then
echo "Error: Config file not found"
exit 1
fi
# Test 2: Find all YAML files that might need to be included
echo "Checking for potential missing YAML files..."
fd -e yml -e yaml --exclude build --exclude tools/yscope-dev-utils/lint-configs/.yamllint.yml \
--exec echo "Found YAML file: {}"
Length of output: 207
Script:
#!/bin/bash
# Let's verify the actual path structure and find YAML files
echo "Repository YAML files:"
find . -type f \( -name "*.yml" -o -name "*.yaml" \) -not -path "*/\.*" -not -path "*/build/*"
echo -e "\nRepository structure for tools directory:"
ls -R tools/
Length of output: 351
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
README.md (1)
17-18: Consider enhancing the YAML file instructions.While the current instruction is clear, it would be helpful to provide an example of how to add a YAML file to the
yamllintcommand.Consider adding an example like:
* **YAML**. If adding a YAML file (regardless of its extension), add it as an argument to the `yamllint` command in [lint-tasks.yaml](lint-tasks.yaml). + For example: + ```yaml + # In lint-tasks.yaml + yamllint -c .yamllint.yml path/to/your-new-file.yml other-files/*.yaml + ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (1 hunks)
🔇 Additional comments (1)
README.md (1)
14-19: LGTM! Clear and well-placed documentation addition.The new section effectively communicates the requirement to manually add YAML files to the linting rules, which aligns perfectly with the PR's objective of using an include list approach.
Description
There are usually more YAML files to exclude (e.g., those in the generated files for a CMake build) than include in a repo, so it's better to select the YAML files to lint using an include list.
Validation performed
task lint:checkdetected it.Summary by CodeRabbit