feat(linting): Add Taskfile tasks to run linters. #76
Conversation
WalkthroughThis pull request involves the deletion of configuration files for Clang tools, specifically Changes
Possibly related PRs
Suggested reviewers
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 (
|
.yamllint.yml
Outdated
| @@ -0,0 +1,51 @@ | |||
| extends: "default" | |||
There was a problem hiding this comment.
straight copy & paste from OSS
tools/yscope-dev-utils
Outdated
There was a problem hiding this comment.
straight copy & paste from OSS
tools/scripts/deps-download/init.sh
Outdated
There was a problem hiding this comment.
straight copy & paste from OSS
There was a problem hiding this comment.
straight copy & paste from OSS
lint-requirements.txt
Outdated
There was a problem hiding this comment.
straight copy & paste from OSS
|
See https://github.com/y-scope/spider for more up-to-date ways of doing things. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (9)
.gitmodules (1)
4-6: Consider documenting the submodule's purpose.Since this submodule appears to be related to development utilities and linting, it would be helpful to document its purpose and any setup requirements in the README.
.yamllint.yml (2)
22-30: Consider enabling document start markers for explicit parsing.While disabling document start/end markers makes files shorter, explicit markers can prevent YAML parser ambiguity, especially in files that might contain multiple documents in the future.
Consider enabling
document-start:document-start: - present: false + present: true
38-40: Consider allowing non-breakable words for URLs.Disallowing non-breakable words might cause issues with long URLs or similar content that shouldn't be broken across lines.
Consider this adjustment:
line-length: max: 100 - allow-non-breakable-words: false + allow-non-breakable-words: truedocs/conf/conf.py (1)
69-71: Add docstring to document the setup function's purpose.The setup function implementation is correct, but adding a docstring would improve maintainability.
Apply this diff to add documentation:
def setup(app): + """Add custom CSS to the Sphinx documentation. + + Args: + app: Sphinx application instance + Returns: + None + """ app.add_css_file("custom.css")tools/scripts/deps-download/download-dep.py (2)
15-21: Consider enhancing the logging format for better debugging.The current logging format could be improved by adding the module name and line number.
-logging_formatter = logging.Formatter("%(asctime)s [%(levelname)s] %(message)s") +logging_formatter = logging.Formatter("%(asctime)s [%(levelname)s] [%(module)s:%(lineno)d] %(message)s")
1-88: Consider architectural improvements for better maintainability.Since this is a critical dependency management script, consider:
- Adding a configuration file for allowlisted domains and default parameters
- Implementing telemetry for tracking failed downloads and debugging issues
- Adding retry mechanisms for network operations
- Creating an error reporting mechanism to track issues in production
lint-tasks.yml (1)
6-17: Add task descriptions for better documentation.Consider adding descriptions to the main tasks to clarify their purpose and behaviour. This helps other developers understand when to use
checkversusfix.tasks: check: + desc: "Run all linting checks without making changes" cmds: - task: "cpp-check" - task: "py-check" - task: "yml-check" fix: + desc: "Automatically fix linting issues across all supported languages" cmds: - task: "cpp-fix" - task: "py-fix" - task: "yml-fix".github/workflows/build_wheels.yml (2)
Line range hint
3-27: Update workflow triggers to reflect removed filesThe path triggers still include
.clang-formatand.clang-tidy, but according to the PR summary, these files are being removed. Consider removing these paths from both thepull_requestandpushtriggers to prevent unnecessary workflow runs.Apply this diff to update the path triggers:
pull_request: paths: - - ".clang-format" - - ".clang-tidy" - ".github/workflows/build_wheels.yml" - ".gitmodules" # ... rest of paths ... push: paths: - - ".clang-format" - - ".clang-tidy" - ".github/workflows/build_wheels.yml" - ".gitmodules" # ... rest of paths ...
Remove duplicate linting steps - task lint:check already includes Python linting
Based on the Taskfile content, the
task lint:checkcommand already includes Python linting through itspy-checksubtask, which runs both black and ruff with appropriate configurations. The current workflow has redundant steps.Recommended changes:
- Remove the standalone Python linting tools as they're already covered by the task-based approach
- Keep only the task-based linting which provides a unified and maintainable solution
- - run: docformatter --check --diff clp_ffi_py tests - - run: black --diff --check clp_ffi_py tests - - run: ruff check --output-format=github clp_ffi_py tests - - run: mypy clp_ffi_py tests - name: "Install task" shell: "bash" run: "npm install -g @go-task/cli" - name: "Run lint task" shell: "bash" run: "task lint:check"🔗 Analysis chain
Line range hint
44-69: Consider consolidating linting toolsThe workflow currently runs multiple Python linting tools directly (docformatter, black, ruff, mypy) and then runs
task lint:check. This might lead to:
- Duplicate linting checks
- Inconsistent configurations
- Increased CI time
Consider moving all linting configurations to the task-based approach for better maintainability.
Let's verify what the task lint:check includes:
Consider consolidating all linting under the task-based approach:
- - run: docformatter --check --diff clp_ffi_py tests - - run: black --diff --check clp_ffi_py tests - - run: ruff check --output-format=github clp_ffi_py tests - - run: mypy clp_ffi_py tests - name: "Install task" shell: "bash" run: "npm install -g @go-task/cli" - name: "Run lint task" shell: "bash" run: "task lint:check"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if task lint:check duplicates existing linting tools # Test: Search for the task definition fd -t f "Taskfile.ya?ml" -X cat {} \; | grep -A 10 "lint:check"Length of output: 66
Script:
#!/bin/bash # Let's try different approaches to find task definitions # Look for Taskfile variations with case-insensitive search fd -t f -i "taskfile" -X cat {} # Also check package.json for potential npm scripts that might define these tasks fd -t f "package.json" -X cat {} # Search for any YAML files that might contain task definitions fd -t f -e yml -e yaml -X cat {}Length of output: 13493
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- .clang-format (0 hunks)
- .clang-tidy (0 hunks)
- .github/workflows/build_wheels.yml (1 hunks)
- .gitmodules (1 hunks)
- .yamllint.yml (1 hunks)
- Taskfile.yml (4 hunks)
- docs/conf/conf.py (1 hunks)
- lint-requirements.txt (1 hunks)
- lint-tasks.yml (1 hunks)
- src/.clang-format (1 hunks)
- tools/scripts/deps-download/download-dep.py (1 hunks)
- tools/scripts/deps-download/init.sh (1 hunks)
- tools/yscope-dev-utils (1 hunks)
💤 Files with no reviewable changes (2)
- .clang-format
- .clang-tidy
✅ Files skipped from review due to trivial changes (4)
- lint-requirements.txt
- src/.clang-format
- tools/scripts/deps-download/init.sh
- tools/yscope-dev-utils
🧰 Additional context used
🪛 yamllint
Taskfile.yml
[error] 122-122: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (12)
.gitmodules (1)
4-6: LGTM! Verify submodule initialization.The new submodule configuration is properly structured and uses a secure HTTPS URL from the same organization.
Let's verify the submodule configuration:
✅ Verification successful
Submodule configuration verified and properly initialized
The verification confirms:
- The GitHub repository
y-scope/yscope-dev-utilsexists and is accessible- The submodule directory is properly initialized at
tools/yscope-dev-utils🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify submodule configuration and initialization # Test 1: Check if the submodule URL is accessible gh repo view y-scope/yscope-dev-utils --json name,url # Test 2: Verify the submodule path exists test -d "tools/yscope-dev-utils" && echo "Submodule directory exists" || echo "Submodule directory missing"Length of output: 231
.yamllint.yml (4)
10-12: LGTM! Good anchor hygiene practices.Forbidding duplicated and unused anchors helps maintain clean and maintainable YAML files.
14-20: LGTM! Consistent spacing rules.The spacing rules for empty braces and brackets are well-defined and consistent with common YAML style guides.
32-33: LGTM! Good numeric value formatting.Requiring numerals before decimal points prevents ambiguous float values like
.5and promotes better readability.
42-51: LGTM! Well-reasoned special value handling.The configuration makes good choices for octal values and has a well-documented exception for truthy keys to support GitHub Actions.
docs/conf/conf.py (1)
69-71: Verify custom.css file existence.Ensure that the custom CSS file exists in the configured static path.
✅ Verification successful
Custom CSS file exists in the expected location
The
custom.cssfile is present atdocs/src/_static/custom.css, which is the correct static path for Sphinx documentation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of custom.css in the static path # Test: Check if custom.css exists in the static directory fd --type f "custom.css" docs/src/_static/Length of output: 70
tools/scripts/deps-download/download-dep.py (1)
87-88: LGTM!The main entry point follows Python best practices.
Taskfile.yml (2)
4-6: Well-organized includes section!The simplified structure with direct references to task files improves maintainability. The addition of lint tasks aligns well with the PR's objective of porting the linter flow.
16-16: Good addition of initialization dependencyAdding the 'init' task as a dependency ensures the build directory exists before running subsequent tasks, preventing potential runtime errors.
lint-tasks.yml (3)
1-4: LGTM! Version and variable declaration are well-structured.The version declaration and virtual environment path variable are appropriately defined.
108-134: LGTM! Virtual environment management is well-implemented.The virtual environment task is properly structured with:
- Checksum validation
- Clear dependency management
- Proper variable handling
19-28: Verify C++ source file patterns for completeness.The C++ source patterns look comprehensive but let's verify no patterns are missing.
✅ Verification successful
All C++ source files are already covered by the existing patterns
The files shown in the output are not actually missed by the current patterns. The grep command's output is misleading because it's showing files that match the extensions but in a different way than the grep pattern expected. Looking at the patterns in the lint-tasks.yml:
"src/clp_ffi_py/**/*.cpp"covers all .cpp files"src/clp_ffi_py/**/*.h"covers all .h files"src/clp_ffi_py/**/*.hpp"covers all .hpp files"src/clp_ffi_py/**/*.inc"covers all .inc filesThe
**glob pattern ensures all subdirectories are included, and each extension is explicitly covered. All the files in the output fall under these patterns:
- Files like
src/clp_ffi_py/Py_utils.cppmatchsrc/clp_ffi_py/**/*.cpp- Files like
src/clp_ffi_py/ir/native/PyMetadata.hppmatchsrc/clp_ffi_py/**/*.hpp🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all C++ related files and compare with patterns echo "Files that might be missed by current patterns:" fd -e cpp -e hpp -e h -e inc . src/clp_ffi_py | grep -vE "/(\.cpp|\.h|\.hpp|\.inc)$"Length of output: 1449
.yamllint.yml
Outdated
| yaml-files: | ||
| - ".clang-format" | ||
| - ".yamllint" | ||
| - "*.yaml" | ||
| - "*.yml" |
There was a problem hiding this comment.
💡 Codebase verification
Remove .clang-format from yaml-files list as the file no longer exists
The .clang-format file has been removed from the repository, so including it in the YAML linting configuration is unnecessary and may cause confusion for future maintainers.
🔗 Analysis chain
Verify the inclusion of .clang-format in yaml-files.
According to the PR summary, Clang-Format configuration is being removed. Including .clang-format in the YAML linting configuration might be unnecessary.
Let's check if .clang-format still exists:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence and format of .clang-format
# Expected: File should not exist if it's being removed
if [ -f ".clang-format" ]; then
echo "File exists:"
cat ".clang-format"
else
echo "File does not exist"
fi
Length of output: 75
| script_path = Path(os.path.realpath(__file__)) | ||
| git_dir = script_path.parent / ".." / ".." / ".." / ".git" | ||
| if git_dir.exists() and git_dir.is_dir(): | ||
| if parsed_args.use_submodule: | ||
| cmd = ["git", "submodule", "update", "--init", "--recursive", str(dest_dir)] | ||
| try: | ||
| subprocess.run(cmd, check=True) | ||
| except subprocess.CalledProcessError: | ||
| logger.exception(f"Failed to update submodule '{dest_dir}'.") | ||
| return -1 | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
Improve robustness of Git operations.
Current implementation has several potential issues:
- Hardcoded path traversal to find .git directory
- No timeout for Git operations
- No handling of Git stderr output
- git_dir = script_path.parent / ".." / ".." / ".." / ".git"
+ # Use git rev-parse to find repository root
+ try:
+ repo_root = subprocess.run(
+ ["git", "rev-parse", "--show-toplevel"],
+ capture_output=True,
+ text=True,
+ timeout=30,
+ check=True
+ ).stdout.strip()
+ git_dir = Path(repo_root) / ".git"
+ except (subprocess.TimeoutExpired, subprocess.CalledProcessError) as e:
+ logger.error(f"Failed to locate git repository: {e}")
+ return -1Committable suggestion was skipped due to low confidence.
| def main(argv): | ||
| args_parser = argparse.ArgumentParser(description="Download dependency.") | ||
| args_parser.add_argument("source_url", help="URL of the source file.") | ||
| args_parser.add_argument("source_name", help="Name of the source file.") | ||
| args_parser.add_argument("dest_dir", help="Output directory for the download.") | ||
| args_parser.add_argument("--extract", action="store_true", help="Extract the source file.") | ||
| args_parser.add_argument( | ||
| "--no-submodule", | ||
| action="store_false", | ||
| dest="use_submodule", | ||
| help="Don't use git submodule update", | ||
| ) | ||
|
|
||
| parsed_args = args_parser.parse_args(argv[1:]) | ||
| source_url = parsed_args.source_url | ||
| source_name = parsed_args.source_name | ||
| dest_dir = Path(parsed_args.dest_dir).resolve() | ||
| extract_source = parsed_args.extract | ||
|
|
There was a problem hiding this comment.
Add input validation for security.
The script should validate:
- URL format and scheme (restrict to http/https)
- Sanitize file names to prevent path traversal
- Verify URL domain against an allowlist
def main(argv):
args_parser = argparse.ArgumentParser(description="Download dependency.")
args_parser.add_argument("source_url", help="URL of the source file.")
args_parser.add_argument("source_name", help="Name of the source file.")
args_parser.add_argument("dest_dir", help="Output directory for the download.")
args_parser.add_argument("--extract", action="store_true", help="Extract the source file.")
args_parser.add_argument(
"--no-submodule",
action="store_false",
dest="use_submodule",
help="Don't use git submodule update",
)
parsed_args = args_parser.parse_args(argv[1:])
+ # Validate URL
+ parsed_url = urllib.parse.urlparse(parsed_args.source_url)
+ if parsed_url.scheme not in ('http', 'https'):
+ logger.error(f"Invalid URL scheme: {parsed_url.scheme}")
+ return -1
+
+ # Sanitize file names
+ if '..' in parsed_args.source_name or parsed_args.source_name.startswith('/'):
+ logger.error("Invalid source name")
+ return -1Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
.github/ISSUE_TEMPLATE/bug-report.yml (2)
2-2: Consider enhancing the description for clarity.The current description "Report software deficiencies" is quite terse. Consider expanding it to be more descriptive and user-friendly.
-description: "Report software deficiencies" +description: "Report bugs, defects, or unexpected behaviour in the software"
20-26: Improve version field guidance with examples.The placeholder could be more helpful with specific examples.
label: "clp-ffi-py version" description: "The release version number or development commit hash that has the bug." - placeholder: "Version number or commit hash" + placeholder: "e.g., v1.2.3 or 8a371dc"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- .github/ISSUE_TEMPLATE/bug-report.yml (1 hunks)
- .github/ISSUE_TEMPLATE/feature-request.yml (1 hunks)
- .github/workflows/build_wheels.yml (2 hunks)
- Taskfile.yml (4 hunks)
- docs/tasks.yml (1 hunks)
- src/.clang-format (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/ISSUE_TEMPLATE/feature-request.yml
- docs/tasks.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build_wheels.yml
- src/.clang-format
🔇 Additional comments (3)
Taskfile.yml (3)
4-6: LGTM: Clean includes structureThe simplified includes structure improves readability and maintainability.
24-25: Previous comment about checksum file duplication still appliesAlso applies to: 69-69, 96-97
118-122: Well-structured initialization taskThe init task is properly configured with appropriate flags and follows task runner best practices.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
lint-tasks.yml (1)
6-18: Add task descriptions and dependencies.Consider enhancing the main task groups with descriptions and appropriate dependencies. This improves maintainability and ensures proper task execution order.
tasks: check: + desc: "Run all linting checks without making changes" + deps: ["venv"] cmds: - task: "cpp-check" - task: "py-check" - task: "yml-check" fix: + desc: "Auto-fix all linting issues where possible" + deps: ["venv"] cmds: - task: "cpp-fix" - task: "py-fix" - task: "yml-fix"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Taskfile.yml (2 hunks)
- lint-tasks.yml (1 hunks)
🔇 Additional comments (6)
Taskfile.yml (4)
4-6: LGTM! Clean includes structure.The simplified includes section improves readability and maintainability.
16-16: LGTM! Good initialization practice.Adding the init task as a dependency ensures the build directory exists before other tasks run.
118-122: Add newline at end of fileThe previous review comment about adding a newline at the end of the file is still applicable.
Line range hint
24-24: Consider reducing duplication in checksum file definitionsThe previous review comment about moving similar CHECKSUM_FILE definitions to a common variable is still valid.
Also applies to: 69-69, 96-96
lint-tasks.yml (2)
1-4: LGTM! Well-structured configuration header.The version declaration and variable definition follow Task conventions properly.
107-133: LGTM! Well-implemented virtual environment management.The virtual environment task implementation includes proper cache invalidation, clear dependency chain, and appropriate variable usage.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
lint-tasks.yml (1)
44-45: Track TODO items in issue trackerThe disabled clang-tidy tasks should be tracked in the issue system for better visibility and follow-up.
Would you like me to create GitHub issues to track these TODOs?
Also applies to: 50-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .gersemirc (1 hunks)
- .github/workflows/build_wheels.yml (2 hunks)
- Taskfile.yml (1 hunks)
- lint-requirements.txt (1 hunks)
- lint-tasks.yml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gersemirc
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build_wheels.yml
- lint-requirements.txt
🔇 Additional comments (6)
Taskfile.yml (2)
4-6: LGTM! Simplified includes structureThe restructured includes section improves clarity and maintainability.
19-19: LGTM! Proper initialization handlingAdding the init dependency ensures the build directory exists before running the wheel task.
lint-tasks.yml (4)
10-24: LGTM: Well-organized task structureThe main task definitions are well-organized with clear separation between check and fix operations for each tool.
105-122: LGTM: Comprehensive Python linting setupThe Python linting configuration properly utilizes both black and ruff for comprehensive code quality checks.
141-219: LGTM: Well-structured internal tasksThe internal tasks are well-structured with proper dependency management and environment activation.
7-8: Review directory structure consistencyThe directory structure appears to have inconsistencies:
CPP_LINT_DIRSusessrc/clp_ffi_pywhile the find commands later use direct pathsPYTHON_LINT_DIRSincludes atestsdirectory that wasn't mentioned in the changes summary✅ Verification successful
Directory Structure in
lint-tasks.ymlis CorrectThe directory structure defined in the lint tasks configuration is accurate and consistent with the actual codebase layout:
PYTHON_LINT_DIRScorrectly includes:
clp_ffi_py/containing Python modulesdocs/conf/containing configurationtests/containing test filesCPP_LINT_DIRSpoints tosrc/clp_ffi_py, which exists as a separate directory🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify directory structure echo "Checking directory structure..." fd -t d "clp_ffi_py" fd -t d "src/clp_ffi_py"Length of output: 508
Script:
#!/bin/bash # Let's verify the actual content and structure more thoroughly echo "Checking Python files in clp_ffi_py..." fd -t f -e py . clp_ffi_py/ echo -e "\nChecking Python files in src/clp_ffi_py..." fd -t f -e py . src/clp_ffi_py/ echo -e "\nChecking if tests directory exists and contains Python files..." fd -t f -e py . tests/ echo -e "\nChecking docs/conf directory..." fd -t f -e py . docs/conf/Length of output: 1038
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
README.md (2)
54-60: Enhance submodule initialization documentationThe Task tool requirement and submodule setup instructions could be more explicit.
Consider adding:
- A note about why the yscope-dev-utils submodule is required
- A verification step to confirm successful submodule initialization
🧰 Tools
🪛 Markdownlint
54-54: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
57-57: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
297-328: Enhance linting documentation clarityThe linting section is well-structured but could benefit from additional clarity.
Consider adding:
- A note about which linters are mandatory vs optional
- Expected execution time for each linting task
- Examples of common linting violations and their fixes
🧰 Tools
🪛 LanguageTool
[style] ~306-~306: Consider using a different verb for a more formal wording.
Context: ...un all linting checks AND automatically fix any fixable issues: ```shell task lint:...(FIX_RESOLVE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (7 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
39-39: Missing link or image reference definition: "14"
Reference links and images should use a label that is defined(MD052, reference-links-images)
54-54: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
57-57: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
291-291: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
293-293: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
294-294: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
331-331: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
332-332: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
335-335: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
336-336: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
🪛 LanguageTool
README.md
[style] ~306-~306: Consider using a different verb for a more formal wording.
Context: ...un all linting checks AND automatically fix any fixable issues: ```shell task lint:...(FIX_RESOLVE)
README.md
Outdated
| ## Adding files | ||
| Certain file types need to be added to our linting rules manually: | ||
|
|
||
| * **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. | ||
| * **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). |
There was a problem hiding this comment.
Standardize list formatting
The list formatting is inconsistent with the rest of the document. Markdown lists should use dashes (-) instead of asterisks (*) for consistency.
Apply this formatting change:
-* **CMake**. If adding a CMake file...
-* **YAML**. If adding a YAML file...
+- **CMake**. If adding a CMake file...
+- **YAML**. If adding a YAML file...Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Markdownlint
291-291: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
293-293: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
294-294: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
README.md
Outdated
| @@ -1,6 +1,6 @@ | |||
| # clp-ffi-py | |||
|
|
|||
| [![PyPI platforms][badge_pypi]][16] | |||
| [![PyPI platforms][badge_pypi]][10] | |||
There was a problem hiding this comment.
💡 Codebase verification
Reference links need to be updated for consistency
Several reference links need adjustments to match their correct definitions:
- Line 3:
[10]currently points to Python's pickle documentation but is used with[badge_pypi] - Line 197:
[12]correctly points to smart_open - Line 244:
[10]correctly points to pickle documentation - Line 246:
[8]and[9]correctly point to multiprocessing and tqdm respectively
Additionally, these issues need attention:
- Missing definition for
[badge_pypi]reference - Missing definition for
[14]reference used for docs hub
🔗 Analysis chain
Fix broken reference links
Several reference links are either missing or incorrectly numbered. Please ensure all reference links are properly defined and sequentially ordered.
Apply these corrections:
- Line 3: Update
[10]to match the correct reference for PyPI platforms - Line 197: Verify
[12]points to smart_open - Line 244-246: Ensure
[10],[8], and[9]point to the correct references
Also applies to: 197-197, 244-244, 246-246
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify all reference links are defined
rg '\[[0-9]+\](?!\()' README.md
Length of output: 263
Script:
#!/bin/bash
# Search for reference-style link definitions
rg '^\[[0-9]+\]:' README.md
# Search for reference-style link usage
rg '\[.*?\]\[[0-9]+\]' README.md
Length of output: 1640
README.md
Outdated
| Additionally, the following tools can be useful during development. | ||
|
|
||
| * [mypy][3]: `mypy clp_ffi_py` | ||
| * mypy checks for typing errors. You should resolve all typing errors or if an | ||
| error cannot be resolved (e.g., it's due to a third-party library), you | ||
| should add a comment `# type: ignore` to [silence][4] the error. | ||
| * [docformatter][11]: `docformatter -i clp_ffi_py tests` | ||
| * [docformatter][6]: `docformatter -i clp_ffi_py tests` | ||
| * This formats docstrings. You should review and add any changes to your PR. | ||
| * [Black][5]: `black clp_ffi_py` | ||
| * This formats the Python code according to Black's code-style rules. You | ||
| should review and add any changes to your PR. | ||
| * [clang-format][6]: `clang-format -i src/clp_ffi_py/**` | ||
| * This formats the C++ code according to the code-style rules specified in | ||
| `.clang-format`. You should review and add any changes to your PR. | ||
| * [ruff][10]: `ruff check --fix clp_ffi_py tests` | ||
| * This performs linting according to PEPs. You should review and add any | ||
| changes to your PR. | ||
| * [gersemi][9]: `gersemi -i -l 100 --list-expansion favour-expansion CMakeLists.txt` | ||
| * This formats CMakeLists.txt. You should review and add any changes to your PR. | ||
|
|
||
| Note that `docformatter` should be run before `black` to give Black the | ||
| [last][12]. | ||
|
|
||
| Additionally, the following tools can be useful during development. However, | ||
| they cannot be installed using `pip`. Developers need to install them using | ||
| other package management tools such as `apt-get`: | ||
|
|
||
| * [clang-tidy][8]: `clang-tidy --extra-arg=-std=c++17 PATH_TO_THE_FILE` | ||
| * This static analysis tool catches improper coding behaviors based on the | ||
| rules specified in `.clang-tidy`, and sends suggestions corresponding to | ||
| each warning. Developers should manually review all the warnings and try | ||
| with their best effort to fix the reasonable ones. | ||
|
|
||
| Note that `docformatter` should be run before `linter` to give Black the | ||
| [last][7]. |
There was a problem hiding this comment.
Clarify development tools usage
The development tools section needs consistent formatting and clearer instructions.
- Fix list formatting:
-* [mypy][3]: `mypy clp_ffi_py`
-* [docformatter][6]: `docformatter -i clp_ffi_py tests`
+- [mypy][3]: `mypy clp_ffi_py`
+- [docformatter][6]: `docformatter -i clp_ffi_py tests`- Update the note about docformatter:
-Note that `docformatter` should be run before `linter` to give Black the
-[last][7].
+Note: Run `docformatter` before running the linting tasks to ensure Black has the
+[final formatting pass][7].📝 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.
| Additionally, the following tools can be useful during development. | |
| * [mypy][3]: `mypy clp_ffi_py` | |
| * mypy checks for typing errors. You should resolve all typing errors or if an | |
| error cannot be resolved (e.g., it's due to a third-party library), you | |
| should add a comment `# type: ignore` to [silence][4] the error. | |
| * [docformatter][11]: `docformatter -i clp_ffi_py tests` | |
| * [docformatter][6]: `docformatter -i clp_ffi_py tests` | |
| * This formats docstrings. You should review and add any changes to your PR. | |
| * [Black][5]: `black clp_ffi_py` | |
| * This formats the Python code according to Black's code-style rules. You | |
| should review and add any changes to your PR. | |
| * [clang-format][6]: `clang-format -i src/clp_ffi_py/**` | |
| * This formats the C++ code according to the code-style rules specified in | |
| `.clang-format`. You should review and add any changes to your PR. | |
| * [ruff][10]: `ruff check --fix clp_ffi_py tests` | |
| * This performs linting according to PEPs. You should review and add any | |
| changes to your PR. | |
| * [gersemi][9]: `gersemi -i -l 100 --list-expansion favour-expansion CMakeLists.txt` | |
| * This formats CMakeLists.txt. You should review and add any changes to your PR. | |
| Note that `docformatter` should be run before `black` to give Black the | |
| [last][12]. | |
| Additionally, the following tools can be useful during development. However, | |
| they cannot be installed using `pip`. Developers need to install them using | |
| other package management tools such as `apt-get`: | |
| * [clang-tidy][8]: `clang-tidy --extra-arg=-std=c++17 PATH_TO_THE_FILE` | |
| * This static analysis tool catches improper coding behaviors based on the | |
| rules specified in `.clang-tidy`, and sends suggestions corresponding to | |
| each warning. Developers should manually review all the warnings and try | |
| with their best effort to fix the reasonable ones. | |
| Note that `docformatter` should be run before `linter` to give Black the | |
| [last][7]. | |
| Additionally, the following tools can be useful during development. | |
| - [mypy][3]: `mypy clp_ffi_py` | |
| * mypy checks for typing errors. You should resolve all typing errors or if an | |
| error cannot be resolved (e.g., it's due to a third-party library), you | |
| should add a comment `# type: ignore` to [silence][4] the error. | |
| - [docformatter][6]: `docformatter -i clp_ffi_py tests` | |
| * This formats docstrings. You should review and add any changes to your PR. | |
| Note: Run `docformatter` before running the linting tasks to ensure Black has the | |
| [final formatting pass][7]. |
🧰 Tools
🪛 Markdownlint
331-331: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
332-332: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
335-335: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
336-336: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
There was a problem hiding this comment.
@LinZhihao-723 any preference on the 2nd comment?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
lint-tasks.yml (3)
4-8: Consider standardizing variable naming convention.The variable names mix different styles:
LINT_VENV_DIRandLINT_VENV_CHECKSUM_FILEuse underscoresPYTHON_LINT_DIRSandCPP_LINT_DIRSappend_DIRSsuffixConsider standardizing to either:
- PYTHON_LINT_DIRS: ["{{.ROOT_DIR}}/clp_ffi_py", "{{.ROOT_DIR}}/tests"] - CPP_LINT_DIRS: ["{{.CLP_FFI_PY_SRC_DIR}}"] + LINT_PYTHON_DIRS: ["{{.ROOT_DIR}}/clp_ffi_py", "{{.ROOT_DIR}}/tests"] + LINT_CPP_DIRS: ["{{.CLP_FFI_PY_SRC_DIR}}"]
40-41: Track TODOs in GitHub issues.There are multiple TODO comments about fixing clang-tidy warnings. These should be tracked in GitHub issues to ensure proper follow-up.
Would you like me to create GitHub issues to track:
- Fixing all clang-tidy warnings
- Re-enabling cpp-static-check and cpp-static-fix tasks
Also applies to: 48-49, 54-55
209-212: Separate linter and dev requirements.The TODO comment indicates a need to split requirements-dev.txt into separate files for linter and development dependencies.
Would you like me to:
- Create a GitHub issue to track this task
- Help generate separate requirements files based on the current requirements-dev.txt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.gitmodules(1 hunks)lint-tasks.yml(1 hunks)requirements-dev.txt(2 hunks)src/.clang-format(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitmodules
- requirements-dev.txt
- src/.clang-format
🧰 Additional context used
📓 Learnings (1)
lint-tasks.yml (2)
Learnt from: haiqi96
PR: y-scope/clp-ffi-py#76
File: lint-tasks.yml:129-146
Timestamp: 2024-11-10T16:46:53.300Z
Learning: When suggesting dynamic file discovery for YAML files in `lint-tasks.yml` using `fd`, be cautious as it may include YAML files from submodules, which may not be desired. Avoid suggestions that could cause linting to target files under submodules unintentionally.
Learnt from: haiqi96
PR: y-scope/clp-ffi-py#76
File: lint-tasks.yml:53-63
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In the `lint-tasks.yml` file, both `src/.clang-format` and `.clang-format` in the root directory are intentionally different and necessary. They should not be considered duplicates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lint-tasks.yml (2)
44-45: Track the TODO for re-enabling cpp-static-check.The comment indicates that cpp-static-check needs to be re-enabled once clang-tidy issues are fixed. This should be tracked to ensure it's not forgotten.
Would you like me to create a GitHub issue to track the re-enabling of cpp-static-check?
205-208: Track the TODO for separating requirements.The comment indicates that requirements-dev.txt should be split into separate files for linter and dev requirements. This separation would improve dependency management.
Would you like me to create a GitHub issue to track the separation of requirements files?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lint-tasks.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
lint-tasks.yml (2)
Learnt from: haiqi96
PR: y-scope/clp-ffi-py#76
File: lint-tasks.yml:129-146
Timestamp: 2024-11-10T16:46:53.300Z
Learning: When suggesting dynamic file discovery for YAML files in `lint-tasks.yml` using `fd`, be cautious as it may include YAML files from submodules, which may not be desired. Avoid suggestions that could cause linting to target files under submodules unintentionally.
Learnt from: haiqi96
PR: y-scope/clp-ffi-py#76
File: lint-tasks.yml:53-63
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In the `lint-tasks.yml` file, both `src/.clang-format` and `.clang-format` in the root directory are intentionally different and necessary. They should not be considered duplicates.
🔇 Additional comments (3)
lint-tasks.yml (3)
1-9: LGTM! Well-structured variable definitions.
The variables are properly scoped and organized, with clear naming conventions and good use of lists for target directories.
10-24: LGTM! Clear and logical task organization.
The main tasks provide a well-structured entry point for both checking and fixing code across all supported languages.
132-149: LGTM! Appropriate YAML linting configuration.
The explicit file listing is the correct approach here, as it avoids unintentionally linting files in submodules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lint-tasks.yml (2)
39-111: Consider tracking the TODOs in the issue tracker.There are two important TODOs that should be tracked:
- Removing cpp-configs once all clang-tidy issues are fixed
- Re-enabling cpp-static-check once clang-tidy issues are fixed
These items should be tracked to ensure they're not forgotten.
Would you like me to create GitHub issues to track these TODOs?
209-212: Consider tracking the requirements separation TODO.The TODO about separating requirements-dev.txt into linter and dev requirements should be tracked to ensure better dependency management.
Would you like me to create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lint-tasks.yml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
lint-tasks.yml (2)
Learnt from: haiqi96
PR: y-scope/clp-ffi-py#76
File: lint-tasks.yml:129-146
Timestamp: 2024-11-10T16:46:53.300Z
Learning: When suggesting dynamic file discovery for YAML files in `lint-tasks.yml` using `fd`, be cautious as it may include YAML files from submodules, which may not be desired. Avoid suggestions that could cause linting to target files under submodules unintentionally.
Learnt from: haiqi96
PR: y-scope/clp-ffi-py#76
File: lint-tasks.yml:53-63
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In the `lint-tasks.yml` file, both `src/.clang-format` and `.clang-format` in the root directory are intentionally different and necessary. They should not be considered duplicates.
🔇 Additional comments (5)
lint-tasks.yml (5)
1-8: LGTM! Well-structured variable definitions.
The variables are clearly defined and follow proper naming conventions. The use of template variables ensures consistency across different environments.
10-24: LGTM! Well-organized task hierarchy.
The main tasks provide a clear separation between checking and fixing operations, with a logical sequence of language-specific tasks.
25-38: LGTM! Clear separation of CMake check and fix tasks.
The tasks are well-structured with proper dependency management.
112-135: LGTM! Comprehensive Python linting setup.
The tasks effectively combine multiple Python linting tools (mypy, black, ruff, docformatter) with appropriate flags for both checking and fixing operations.
136-153: LGTM! Appropriate YAML linting configuration.
The explicit listing of YAML files and directories is the correct approach, as it prevents unintended linting of files in submodules.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
Taskfile.yml (1)
4-6: Consider adding comments to document included task filesThe includes section is well-organized, but adding brief comments about each included file's purpose would improve maintainability.
includes: + # Tasks for building and managing documentation docs: "docs/tasks.yml" + # Linting and code quality tasks lint: "lint-tasks.yml" + # Common utility tasks utils: "tools/yscope-dev-utils/taskfiles/utils.yml"lint-tasks.yml (2)
39-44: Consider tracking the temporary dependency.The TODO comment and dependency on
:config-cmake-projectare acknowledged as temporary. Consider creating a tracking issue to revisit this once clang-tidy warnings are fixed.Would you like me to create a GitHub issue to track the cleanup of this temporary dependency?
111-120: Add GitHub output format for ruff.To maintain consistency with the current workflow check, consider adding the GitHub output format flag for ruff.
- RUFF_FLAGS: "" + RUFF_FLAGS: "--output-format=github"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
README.md(7 hunks)Taskfile.yml(1 hunks)lint-tasks.yml(1 hunks)src/clp_ffi_py/.clang-format(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/clp_ffi_py/.clang-format
🧰 Additional context used
📓 Learnings (1)
Taskfile.yml (1)
Learnt from: haiqi96
PR: y-scope/clp-ffi-py#76
File: Taskfile.yml:11-13
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `Taskfile.yml`, variables no longer use the `G_` prefix. For example, `G_BUILD_DIR` has been renamed to `BUILD_DIR`.
🪛 Markdownlint (0.35.0)
README.md
54-54: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
57-57: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
293-293: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
🪛 LanguageTool
README.md
[style] ~306-~306: Consider using a different verb for a more formal wording.
Context: ...un all linting checks AND automatically fix any fixable issues: ```shell task lint:...
(FIX_RESOLVE)
🔇 Additional comments (9)
Taskfile.yml (2)
20-20: LGTM! Good addition of initialization dependency
Adding the init task as a dependency ensures proper directory setup before running the wheel task.
39-43: 🛠️ Refactor suggestion
Add error handling for directory creation
The init task should handle potential directory creation failures gracefully.
- cmds: ["mkdir -p '{{.BUILD_DIR}}'"]
+ cmds:
+ - |
+ if ! mkdir -p '{{.BUILD_DIR}}'; then
+ echo "Error: Failed to create build directory"
+ exit 1
+ fiLikely invalid or redundant comment.
lint-tasks.yml (2)
1-9: LGTM! Well-structured variable declarations.
The global variables are well-organized with clear prefixes and proper documentation.
10-24: LGTM! Clear task organization.
The main tasks are well-structured with a clear separation between check and fix operations.
README.md (5)
3-3: LGTM! Links are properly updated and consistent.
All badge and reference links are correctly defined and consistently used throughout the document.
Also applies to: 30-30, 39-39
56-60: LGTM! Clear submodule setup instructions.
The instructions for initializing and updating the yscope-dev-utils submodules are clear and accurate.
🧰 Tools
🪛 Markdownlint (0.35.0)
57-57: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
297-327: LGTM! Comprehensive linting documentation.
The linting section is well-structured with:
- Clear instructions for running checks
- Detailed task table with descriptions
- Proper guidance for specific linter usage
🧰 Tools
🪛 LanguageTool
[style] ~306-~306: Consider using a different verb for a more formal wording.
Context: ...un all linting checks AND automatically fix any fixable issues: ```shell task lint:...
(FIX_RESOLVE)
331-338: LGTM! Reference links are properly organized.
All reference links are correctly defined and logically ordered.
54-54: Verify Task tool version requirement
Please ensure that the Task tool version 3.38.0 is the minimum required version for all the new linting tasks.
🧰 Tools
🪛 Markdownlint (0.35.0)
54-54: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
54-54: Standardize list formattingFor consistency with the rest of the document, use dashes instead of asterisks for list items.
-* [Task][9] >= 3.38.0 +- [Task][9] >= 3.38.0🧰 Tools
🪛 Markdownlint (0.35.0)
54-54: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
57-60: Standardize list formatting and improve command presentationUpdate the list formatting for consistency and enhance the command presentation.
-* Initialize and update yscope-dev-utils submodules: +- Initialize and update yscope-dev-utils submodules:🧰 Tools
🪛 Markdownlint (0.35.0)
57-57: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
README.md(7 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
README.md
54-54: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
57-57: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
🪛 LanguageTool
README.md
[style] ~305-~305: Consider using a different verb for a more formal wording.
Context: ...un all linting checks AND automatically fix any fixable issues: ```shell task lint:...
(FIX_RESOLVE)
🔇 Additional comments (4)
README.md (4)
3-3: LGTM! Link references are consistent.
The updated reference links correctly point to their respective destinations.
Also applies to: 30-30, 39-39
244-246: LGTM! References are properly updated.
The library references are correctly updated and maintain consistency with the reference definitions.
330-337: LGTM! Reference definitions are well-organized.
The reference definitions are properly ordered and consistent with their usage throughout the document.
319-320:
Verify cpp-static-check/fix task documentation
Based on the previous discussion in PR comments, the cpp-static-check/fix tasks should be temporarily removed until all warnings are fixed.
-| `lint:cpp-check` | Runs the C++ linters (formatters and static analyzers). |
-| `lint:cpp-fix` | Runs the C++ linters and fixes some violations. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lint-tasks.yml (2)
60-62: Consider removing unused file extensions.Based on the project guidelines,
.incfiles are not used and not planned to be added.- "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.cpp" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.h" - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.hpp" - - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.inc"
194-197: Consider tracking the requirements separation task.The TODO comment about separating requirements-dev.txt into linter & dev requirements represents a good future improvement.
Would you like me to create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lint-tasks.yml(1 hunks)
🔇 Additional comments (5)
lint-tasks.yml (5)
1-9: LGTM! Well-structured variable declarations.
The global variables follow consistent naming conventions and use proper templating for paths.
10-24: LGTM! Clear task organization.
The main tasks are well-organized into check and fix categories with proper subtasks for each language.
25-38: LGTM! Well-structured CMake tasks.
The CMake tasks are properly configured with appropriate flags for check and fix operations.
111-130: LGTM! Comprehensive Python linting setup.
The Python tasks properly integrate multiple linting tools with appropriate flags for both check and fix operations.
131-148: LGTM! Proper YAML linting configuration.
The YAML tasks are well-configured with explicit file paths, which is the preferred approach to avoid linting files in submodules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
lint-tasks.yml (3)
3-8: Consider adding more descriptive comments for global variables.The variables section would benefit from additional documentation explaining the purpose and usage of each global variable, particularly
G_LINT_VENV_DIRandG_LINT_VENV_CHECKSUM_FILE.vars: + # Directory for the linting virtual environment G_LINT_VENV_DIR: "{{.BUILD_DIR}}/lint-venv" + # Checksum file to track changes in the virtual environment G_LINT_VENV_CHECKSUM_FILE: "{{.BUILD_DIR}}/lint#venv.md5" # Linter target dirs + # C++ source directories to be linted G_CPP_LINT_DIRS: ["{{.CLP_FFI_PY_CPP_SRC_DIR}}"] + # Python source directories to be linted G_PYTHON_LINT_DIRS: ["{{.ROOT_DIR}}/clp_ffi_py", "{{.ROOT_DIR}}/tests"]
170-170: Remove hardcoded line length.Since the task is running under the root directory (using
dir: "{{.ROOT_DIR}}") where pyproject.toml is located, the line length configuration should be moved to pyproject.toml instead of being hardcoded.- black --color --line-length 100 {{.BLACK_FLAGS}} {{.ITEM}} + black --color {{.BLACK_FLAGS}} {{.ITEM}}
194-197: Consider separating requirements now.The TODO comments suggest separating linter and dev requirements. This separation would improve maintainability and reduce the virtual environment size.
Would you like me to help create separate requirements files for linter and dev dependencies?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lint-tasks.yml(1 hunks)
🔇 Additional comments (3)
lint-tasks.yml (3)
131-147: LGTM!
The YAML linting configuration is appropriate, with explicit file listing to avoid unintended linting of submodule files.
153-155: Verify CMake file existence before formatting.
The gersemi command assumes the existence of CMakeLists.txt and the source directory. Consider adding a check to ensure these paths exist.
58-65: 🛠️ Refactor suggestion
Remove unused .inc extension from source files.
According to previous discussions, this project doesn't use .inc files and isn't planning to add them according to the latest guidelines.
sources: &cpp_format_src_files
- "{{.G_LINT_VENV_CHECKSUM_FILE}}"
- "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.cpp"
- "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.h"
- "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.hpp"
- - "{{.CLP_FFI_PY_CPP_SRC_DIR}}/**/*.inc"
- "src/clp_ffi_py/.clang-format"
- "{{.TASKFILE}}"
- ".clang-format"Likely invalid or redundant comment.
haiqi96
changed the title
References
Description
Port latest task-based linter flow into clp ffi py. Made a few customizations:
Validation performed
Confirmed that linter runs locally.
Confirmed that git workflow's linter job still passes,
Summary by CodeRabbit
New Features
tools/yscope-dev-utilsfor enhanced utility functions..gersemircfor YAML linting with specific settings.Bug Fixes
Documentation
README.mdwith updated links, compatibility notes, and new sections on linting and file management.Chores
requirements-dev.txtto includeclang-tidyandyamllint.Taskfile.ymlfor better organization and clarity.