feat(lint): Add tombi to lint and format TOML files. - #176
Conversation
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughA TOML linting and formatting tool, Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Lint Task Runner
participant Tombi (venv)
participant File System
Developer->>Lint Task Runner: Run "check" or "fix" meta-task
Lint Task Runner->>Tombi (venv): Activate virtualenv
Lint Task Runner->>Tombi (venv): Run "tombi lint" or "tombi format"
Tombi (venv)->>File System: Lint/format TOML files per tombi.toml config
File System-->>Tombi (venv): Return results
Tombi (venv)-->>Lint Task Runner: Report lint/format status
Lint Task Runner-->>Developer: Show results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (3)
tests/integration/__init__.py (1)
1-1: Doc-string could be more descriptive.A single-line module doc-string is fine, but across the repo we normally include a short sentence plus an empty line and a longer explanation. Consider expanding to stay consistent with other integration modules.
-"""Integration tests for Spider C++ projects.""" +""" +Top-level package for Spider integration tests. + +This package hosts high-level integration tests that exercise the compiled +C++ binaries and their Python orchestration layer. +"""tests/integration/utils.py (2)
1-2: Minor grammatical tweak.Plural noun aligns better with the helper’s purpose.
-"""Utilities for the network port.""" +"""Utilities for network ports."""
5-6: Type alias may be over-scoped.
AddrTypeis used only once inside this module. Unless you expect it to be reused by other helpers, an inlinetuple[str, int]annotation keeps the file lighter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lint-tasks.yaml(5 hunks)lint-test-requirements.txt(1 hunks)mypy.ini(1 hunks)ruff.toml(1 hunks)test-requirements.txt(0 hunks)test-tasks.yaml(2 hunks)tests/integration/__init__.py(1 hunks)tests/integration/client.py(7 hunks)tests/integration/test_client.py(3 hunks)tests/integration/test_scheduler_worker.py(11 hunks)tests/integration/test_signal.py(6 hunks)tests/integration/utils.py(1 hunks)tombi.toml(1 hunks)tools/yscope-dev-utils(1 hunks)
💤 Files with no reviewable changes (1)
- test-requirements.txt
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: There may be discrepancies in publicly available documentation about tombi TOML linter rule identifiers. When in doubt, defer to the project maintainer's knowledge of the official documentation and test the configuration to ensure it works correctly.
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: In tombi TOML linter configuration, the correct rule identifier for checking empty keys is `key-empty`. The command `tombi --help rules` does not exist. The official documentation is available at https://tombi-toml.github.io/tombi/docs/configuration/.
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: The tombi TOML linter uses `key-empty` as the correct rule identifier for checking empty keys. This can be verified by testing with `tombi lint` command, which will successfully process configurations using this rule identifier. The rule `keys-empty` does not exist in tombi.
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: In tombi TOML linter configuration, the correct rule identifier for checking empty keys is `key-empty`, not `keys-empty`. This is documented at https://tombi-toml.github.io/tombi/docs/configuration/.
📚 Learning: documentation should be added to new functions in the spider codebase, as already discussed with the...
Learnt from: davidlion
PR: y-scope/spider#100
File: src/spider/worker/worker.cpp:205-230
Timestamp: 2025-04-09T17:15:24.552Z
Learning: Documentation should be added to new functions in the spider codebase, as already discussed with the user.
Applied to files:
tests/integration/__init__.py
📚 Learning: in the spider project, external tools like `uv`, python, and task are documented as requirements in ...
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: build-tasks.yaml:21-45
Timestamp: 2025-08-03T01:49:27.764Z
Learning: In the spider project, external tools like `uv`, Python, and Task are documented as requirements in README.md and are expected to be pre-installed by users rather than bootstrapped by the build system.
Applied to files:
tests/integration/__init__.py
📚 Learning: there may be discrepancies in publicly available documentation about tombi toml linter rule identifi...
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: There may be discrepancies in publicly available documentation about tombi TOML linter rule identifiers. When in doubt, defer to the project maintainer's knowledge of the official documentation and test the configuration to ensure it works correctly.
Applied to files:
tombi.tomlruff.tomllint-tasks.yaml
📚 Learning: in tombi toml linter configuration, the correct rule identifier for checking empty keys is `key-empt...
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: In tombi TOML linter configuration, the correct rule identifier for checking empty keys is `key-empty`. The command `tombi --help rules` does not exist. The official documentation is available at https://tombi-toml.github.io/tombi/docs/configuration/.
Applied to files:
tombi.tomllint-tasks.yaml
📚 Learning: in tombi toml linter configuration, the correct rule identifier for checking empty keys is `key-empt...
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: In tombi TOML linter configuration, the correct rule identifier for checking empty keys is `key-empty`, not `keys-empty`. This is documented at https://tombi-toml.github.io/tombi/docs/configuration/.
Applied to files:
tombi.tomllint-tasks.yaml
📚 Learning: the tombi toml linter uses `key-empty` as the correct rule identifier for checking empty keys. this ...
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: tombi.toml:7-10
Timestamp: 2025-08-03T02:01:57.863Z
Learning: The tombi TOML linter uses `key-empty` as the correct rule identifier for checking empty keys. This can be verified by testing with `tombi lint` command, which will successfully process configurations using this rule identifier. The rule `keys-empty` does not exist in tombi.
Applied to files:
tombi.tomllint-tasks.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (25)
mypy.ini (1)
1-11: Confirm CI image ships the same mypy version.
strict = trueturns on all current and future strictness flags. When mypy 1.13 lands it may add new strict flags that break the build unexpectedly. Two options:
- Pin
mypyto a~=1.12range inlint-test-requirements.txt.- Swap to explicit flags so unexpected new ones don’t appear.
Please verify which approach the team prefers before merging.
tools/yscope-dev-utils (1)
1-1: Submodule SHA Verified on RemoteThe submodule URL and commit
b965f19f3e11c548f32bf7ab8e28ff38906be018have been confirmed present in theyscope-dev-utilsrepository. CI will initialise the submodule correctly—no further action required.test-tasks.yaml (1)
59-59: LGTM! Dependency consolidation improves consistency.The change from
test-requirements.txttolint-test-requirements.txtaligns with the consolidated dependency management approach, ensuring both linting and testing environments use the same requirements file.Also applies to: 73-73
tombi.toml (1)
1-35: LGTM! Well-configured TOML linting setup.The tombi configuration is comprehensive and correctly structured:
- Proper file inclusion/exclusion patterns targeting all
.tomlfiles while excluding build artifacts- Correct rule identifiers (including
key-emptyas confirmed by project knowledge)- Schema validation enabled with appropriate catalog sources
- Clear mappings for different TOML file types
Based on retrieved learnings, the
key-emptyrule identifier is correct for tombi.ruff.toml (1)
3-36: LGTM! Comprehensive linting configuration enhances code quality.The expanded ruff configuration significantly improves code quality enforcement:
- Enables all lint codes with selective, well-justified ignores
- Per-file ignores appropriately handle integration test requirements (assert usage, subprocess calls)
- Docstring formatting configuration maintains consistency with line length settings
- The ignore list strikes a good balance between strictness and practicality
lint-tasks.yaml (4)
14-14: LGTM! TOML linting integration enhances code quality.Adding TOML linting tasks to the check and fix workflows ensures comprehensive file format coverage alongside existing C++, Python, and YAML linting.
Also applies to: 21-21
116-117: LGTM! Improved Python linting with mypy integration.The refactoring separates ruff check and format flags for better control, and the addition of mypy enhances type safety enforcement. The variable structure is cleaner and more maintainable.
Also applies to: 122-122, 127-129, 135-137
159-180: LGTM! Well-structured TOML linting implementation.The TOML linting tasks are properly structured with separate lint and format operations, following the established pattern of other linting tools in the project.
205-205: LGTM! Dependency consolidation improves consistency.Switching to
lint-test-requirements.txtaligns with the consolidated dependency management approach across linting and testing environments.Also applies to: 219-219
tests/integration/test_client.py (4)
1-1: LGTM! Excellent documentation and type safety improvements.The addition of comprehensive docstrings and type annotations significantly enhances code clarity and maintainability. The documentation follows proper Python docstring conventions and provides clear parameter/return value descriptions.
Also applies to: 17-22, 54-58, 72-72, 76-79
16-16: LGTM! Proper type annotations for subprocess handling.The return type annotation
tuple[subprocess.Popen[bytes], ...]and fixture return typeGenerator[None, None, None]correctly specify the types, improving type safety and IDE support.Also applies to: 53-53
50-69: LGTM! Improved fixture lifecycle management.The use of
yieldfor fixture lifecycle management is a best practice that ensures proper cleanup of subprocesses after tests complete. The explicit no-op usage of thestorageparameter avoids linting warnings while maintaining the fixture dependency.
87-87: LGTM! Safer subprocess execution.Using
subprocess.runwithcheck=Trueis safer than manually checking the return code, as it will raise an exception immediately if the process fails, providing clearer error reporting.tests/integration/test_signal.py (4)
30-37: Excellent type safety improvement with PopenOpts TypedDict.The introduction of
PopenOptsas aTypedDictprovides clear typing for subprocess options while maintaining flexibility withtotal=False. This improves code readability and type checking.
39-48: Well-structured function signature and comprehensive docstring.The explicit return type annotation
tuple[subprocess.Popen[bytes], subprocess.Popen[bytes]]is precise and matches the actual return statement. The docstring follows a consistent format with clear parameter descriptions.
80-88: Good practice handling unused parameter with explicit ignore.Using
_ = storage # Avoid ARG001is a clean way to handle the unused parameter while maintaining the fixture signature consistency. The generator typing is correct and the docstring is informative.
102-117: Comprehensive test method documentation and typing.The detailed docstring clearly explains the test scenario, expected behaviour, and parameters. The type annotations for parameters and return type enhance code clarity and IDE support.
tests/integration/client.py (4)
74-78: Excellent type alias for MySQL connection abstraction.The
SQLConnectiontype alias cleanly handles the union of possible MySQL connection types, improving code readability throughout the module while maintaining type safety.
80-91: Improved error handling with informative ValueError.The enhanced error message now includes the invalid URL string, making debugging easier when connection failures occur. The function signature and docstring are well-structured.
146-156: Good SQL query formatting for readability.Breaking the SQL INSERT statement across multiple lines improves readability while maintaining functionality. The multiline string formatting is clean and consistent.
203-204: Proper type casting for database results.Using
castwith explicit tuple typing ensures type safety when handling database query results, improving static analysis and IDE support.tests/integration/test_scheduler_worker.py (4)
32-40: Clear function signature and comprehensive docstring.The explicit return type annotation and detailed docstring improve code clarity. The parameter descriptions are concise yet informative.
67-76: Proper fixture typing with generator pattern.Converting to a generator fixture with explicit typing while handling the unused
storageparameter consistently follows the established pattern across integration tests.
87-96: Well-documented fixture with clear lifecycle description.The docstring clearly explains the fixture's purpose, what it yields, and its cleanup behaviour. The generator typing is appropriate for the fixture pattern.
265-276: Consistent test method enhancement pattern.The
@pytest.mark.usefixtures("scheduler_worker")decorator, comprehensive docstring, and explicit type annotations follow the established pattern, improving test clarity and maintainability.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Two more TODOs;
- As we discussed offline, let's split the check mode and the fix mode, so that the check mode can report errors properly.
- We need to update the main README with this new lint task.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
One more comment before approval.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(lint): Add tombi to lint and format TOML files.
2 participants
Description
Note
This PR depends on #174.
This PR
tombito lint requirements.tombi.Checklist
breaking change.
Validation performed
tombion all toml files.Summary by CodeRabbit