build: Rename C++ build tasks to add cpp- prefix. - #178
Conversation
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
|
Warning Rate limit exceeded@sitaowang1998 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces TOML linting and formatting support, adds and refines type annotations in integration tests, consolidates Python and test dependencies, and updates build/test task configurations accordingly. It also introduces new configuration files for Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Lint as TOML Lint Task
participant Tombi as tombi Tool
Dev->>Lint: Run toml-check/toml-fix
Lint->>Tombi: Run tombi lint/format on .toml files
Tombi-->>Lint: Lint/format results
Lint-->>Dev: Report outcome
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
✨ 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: 0
🧹 Nitpick comments (2)
mypy.ini (1)
10-11: Narrow the blanket import ignore?
ignore_missing_imports = truefor the wholemsgpack.*tree can mask real typing errors inside wrapper utilities built around that library. If only a subset of sub-modules causes issues, consider narrowing the pattern or supplying stubs instead.lint-test-requirements.txt (1)
6-10: Version floor added; consider an upper pinAll new lint deps (
mypy,tombi) are pinned only with a minimum version. Both tools occasionally ship breaking changes under a minor bump. Adding,<next-majorguards (e.g.,mypy>=1.12,<2) helps keep CI green.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
build-tasks.yaml(2 hunks)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(3 hunks)tests/integration/client.py(11 hunks)tests/integration/test_client.py(2 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)
💤 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.
📚 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:
ruff.tomltombi.tomllint-tasks.yaml
📚 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:
test-tasks.yaml
📚 Learning: in task build systems, glob patterns like "python/**/*" properly match files at any nested level, in...
Learnt from: sitaowang1998
PR: y-scope/spider#172
File: build-tasks.yaml:26-29
Timestamp: 2025-08-03T01:54:03.099Z
Learning: In Task build systems, glob patterns like "python/**/*" properly match files at any nested level, including package-level pyproject.toml files in subdirectories like "python/spider/client/pyproject.toml". The "**" wildcard recursively matches any number of directory levels.
Applied to files:
test-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
🧬 Code Graph Analysis (3)
tests/integration/test_client.py (1)
tests/integration/client.py (1)
storage(118-127)
tests/integration/test_scheduler_worker.py (1)
tests/integration/client.py (3)
storage(118-127)TaskGraph(51-56)Task(39-47)
tests/integration/test_signal.py (2)
tests/integration/client.py (4)
get_task_outputs(189-214)get_task_state(217-231)remove_job(234-244)storage(118-127)tests/integration/test_scheduler_worker.py (1)
start_scheduler_worker(32-66)
⏰ 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-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (36)
ruff.toml (2)
7-23: Whitespace-only change looks goodThe re-indentation matches the style that
tombi formatwill enforce, so no functional impact.
29-31: Consistent indentation preservedSame remark here—indentation normalised, no further action.
mypy.ini (1)
1-9: Strict mode enabled – great stepTurning on
strict = truewill surface many latent typing issues. Output options also improve readability.lint-test-requirements.txt (1)
12-14: Confirm runtime packages are really test-only
mysql-connector-pythonpulls native libs and can bloat the test venv. Double-check it is not needed at runtime so that production images stay slim.build-tasks.yaml (1)
4-5: No remaining:build:targetreferences detected
Ranrg --fixed-strings ':build:target' --glob '!test-tasks.yaml' --glob '!build-tasks.yaml'across the repo and found no matches.test-tasks.yaml (2)
30-40: Dep rename propagated correctlyDependencies now point at
:build:cpp-target; matches the new task names.
59-74: No lingering references to the removed file
A global grep fortest-requirements.txtonly matched the expected occurrences oflint-test-requirements.txtin:
- test-tasks.yaml
- lint-tasks.yaml
No stray mentions of the old
test-requirements.txtremain.lint-tasks.yaml (4)
14-14: LGTM! TOML tasks properly integrated into main workflows.The addition of
toml-checkandtoml-fixto the maincheckandfixtasks ensures TOML files will be consistently linted and formatted as part of the standard development workflow.Also applies to: 21-21
135-135: LGTM! MyPy integration enhances type checking.Adding
mypyto the Python linting workflow for integration tests will help catch type-related issues early and improve code quality.
159-180: LGTM! TOML task structure follows established patterns.The TOML task definitions properly follow the established pattern with:
- Separate lint and format subtasks
- Proper dependency on the virtual environment
- Consistent aliasing structure matching other tool configurations
205-205: Dependency consolidation verified
lint-test-requirements.txt contains all expected lint (clang-format, clang-tidy, ruff, mypy, gersemi, yamllint, tombi) and test dependencies; the old lint-requirements.txt has been removed; test-tasks.yaml references have been updated.• lint-test-requirements.txt (root) includes all key dependencies
• Old lint-requirements.txt is no longer present
• test-tasks.yaml lines 59 and 73 now point to lint-test-requirements.txtNo further action required.
tombi.toml (4)
1-6: LGTM! File configuration is well-structured.The file inclusion pattern covers all TOML files while properly excluding build artifacts and external tools, which is appropriate for the project structure.
9-13: LGTM! Lint rules are correctly configured.The lint rule configuration uses the correct identifiers:
key-empty(notkeys-empty) as confirmed by the retrieved learningsdotted-keys-out-of-orderandtables-out-of-orderfor consistent formatting- Warning level is appropriate for these style-related rules
15-22: LGTM! Schema validation properly configured with fallback.The schema configuration includes both a local tombi schema catalog and an HTTPS fallback, ensuring robust schema validation even if one source is unavailable.
24-34: LGTM! Schema mappings cover common TOML files.The schema mappings appropriately cover:
tombi.tomlwith the tombi-specific schemapyproject.tomlwith the Python project schemaruff.tomlwith the ruff configuration schemaThis ensures proper validation for the most common TOML configuration files in Python projects.
tests/integration/utils.py (2)
5-5: LGTM! Type alias improves code clarity.The
AddrTypealias clearly documents the expected structure of socket address tuples and enhances type safety throughout the module.
12-13: LGTM! Explicit typing enhances type safety.The explicit typing of
addrasAddrTypebefore indexing improves type checking and makes the code more readable, while maintaining the same functionality.tests/integration/test_client.py (3)
10-10: LGTM! Import consolidation improves consistency.The switch to importing
SQLConnectionfrom.clientstandardizes the connection type across integration tests and reduces direct dependency on the specific MySQL connector implementation.
16-16: LGTM! More specific subprocess typing improves type safety.The refined return type
subprocess.Popen[bytes]is more specific than the genericsubprocess.Popenand accurately reflects that the subprocess streams are in bytes mode.
55-55: LGTM! Consistent type annotations and documentation.The parameter type change to
SQLConnectionaligns with the standardised connection typing, and the added parameter documentation improves code clarity.Also applies to: 61-61
tests/integration/test_signal.py (5)
10-10: LGTM! TypedDict improves subprocess configuration typing.The introduction of
PopenOptsas aTypedDictprovides explicit typing for subprocess configuration options, making the code more maintainable and self-documenting.Also applies to: 30-37
20-20: LGTM! Consistent connection type standardisation.The switch to
SQLConnectionfrom.clientmaintains consistency with other integration test modules and reduces coupling to the specific MySQL connector implementation.
41-41: LGTM! Precise subprocess typing enhances type safety.The refined return type
subprocess.Popen[bytes]accurately reflects the subprocess stream mode and provides better type checking than the genericsubprocess.Popen.
53-57: LGTM! Well-structured subprocess configuration.The use of the
PopenOptsTypedDict for subprocess configuration is clean and maintainable, explicitly defining the options passed tosubprocess.Popen.
84-85: LGTM! Consistent type annotations across test methods.The updated type annotations for
storageandscheduler_worker_signalparameters are consistent with the standardisedSQLConnectiontype and specificsubprocess.Popen[bytes]typing used throughout the file.Also applies to: 109-110, 186-187
tests/integration/test_scheduler_worker.py (6)
22-22: LGTM: Import change aligns with type system improvements.The import of
SQLConnectionfrom the client module provides better type safety by using a unified type alias for MySQL connections.
34-34: LGTM: Subprocess type annotation improvement.The refined return type
tuple[subprocess.Popen[bytes], subprocess.Popen[bytes]]provides better type clarity by specifying the generic parameter forPopen, which helps with static type analysis.
71-71: LGTM: Consistent type annotation update.The parameter type change from the MySQL connection generator to
SQLConnectionaligns with the unified type alias introduced in the client module, improving consistency across the integration tests.Also applies to: 77-77
93-93: LGTM: Fixture parameter type consistency.The
SQLConnectiontype annotation maintains consistency with other fixture parameters and the unified type system.
160-160: LGTM: Consistent fixture parameter typing.All fixture functions now use the
SQLConnectiontype alias consistently, which improves type safety and maintainability across the test suite.Also applies to: 191-191, 231-231
276-277: LGTM: Test method parameter type improvements.The test method parameters now use:
SQLConnectioninstead of the MySQL connection generator type- Direct fixture return types (e.g.,
Task,tuple[TaskGraph, Task, Task, Task]) instead of generator wrappersThese changes improve type clarity and align with the fixture implementations that yield the actual objects rather than generators.
Also applies to: 306-307, 323-324, 343-344
tests/integration/client.py (5)
7-7: LGTM: Import addition for type casting.The
castimport from typing is appropriately added to support explicit type casting for database query results.
74-77: LGTM: Well-designed type alias for MySQL connections.The
SQLConnectiontype alias effectively unifies the two MySQL connection types (MySQLConnectionAbstractandPooledMySQLConnection) that can be returned bymysql.connector.connect(). This provides better type safety while accommodating the different connection implementations.
80-80: LGTM: Consistent function signature updates.All database functions now use the
SQLConnectiontype alias instead of the specificmysql.connector.MySQLConnectiontype. This change provides better type safety and consistency across the client module.Also applies to: 118-118, 130-130, 189-189, 217-217, 234-234, 247-247, 261-261, 280-280
203-204: LGTM: Explicit type casting improves type safety.The explicit
casttolist[tuple[str, str | None, bytes | None]]clarifies the expected structure of the fetched rows from the database query. This helps with static type analysis and makes the code more robust.
227-227: LGTM: Type casting for single row result.The
casttotuple[str]for thefetchone()result appropriately handles the expected single-row, single-column result from the state query. This prevents potentialNonetype issues and clarifies the expected data structure.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
build: Rename C++ build tasks to add `cpp-` prefix.
cpp- prefix.
2 participants
Description
Note
This PR depends on #176.
This PR renames C++ build tasks with
cpp-prefix to avoid name collision with future build tasks for other languages.Checklist
breaking change.
Validation performed
Summary by CodeRabbit