fix: Add lint:py-fix as a sub-task of lint:fix; Update README to include all lint tasks. - #181
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 18 minutes and 29 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)
WalkthroughThe documentation and configuration for linting tasks were updated. The README now lists additional Python linting commands ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
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: 1
🧹 Nitpick comments (2)
requirements-dev.txt (1)
6-8: Consider pinningtombito a stable minor to avoid unexpected breakages
>=0.4.35guarantees the features you need (e.g.,tombi://catalog URLs), but future 0.x releases can introduce breaking changes without a major-version bump. Pinning to~0.4.35or>=0.4.35,<0.5would give you the new functionality while limiting risk.lint-tasks.yaml (1)
171-188: Minor duplication but harmless
toml-fixrunstombi linteven thoughtombi formatwill re-lint after formatting. Dropping the first call would shave seconds off CI without losing coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)lint-tasks.yaml(2 hunks)requirements-dev.txt(1 hunks)ruff.toml(1 hunks)tests/integration/utils.py(0 hunks)tombi.toml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/utils.py
🧰 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: 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/.
Learnt from: sitaowang1998
PR: y-scope/spider#180
File: tombi.toml:17-22
Timestamp: 2025-08-06T13:48:53.787Z
Learning: In tombi TOML linter version 0.4.35 and later, the pseudo-scheme "tombi://" supports external host references like "tombi://json.schemastore.org/api/json/catalog.json" in addition to built-in identifiers. This expanded functionality allows referencing external schema catalogs using the tombi:// scheme, which was not supported in earlier versions.
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.
📚 Learning: ensure to carefully check yaml indentation before suggesting fixes, and avoid removing comments unle...
Learnt from: kirkrodrigues
PR: y-scope/spider#1
File: .github/workflows/pr-title.yaml:11-15
Timestamp: 2024-10-23T02:23:12.161Z
Learning: Ensure to carefully check YAML indentation before suggesting fixes, and avoid removing comments unless necessary.
Applied to files:
ruff.toml
📚 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.tomlREADME.mdtombi.tomllint-tasks.yaml
📚 Learning: in tombi toml linter version 0.4.35 and later, the pseudo-scheme "tombi://" supports external host r...
Learnt from: sitaowang1998
PR: y-scope/spider#180
File: tombi.toml:17-22
Timestamp: 2025-08-06T13:48:53.787Z
Learning: In tombi TOML linter version 0.4.35 and later, the pseudo-scheme "tombi://" supports external host references like "tombi://json.schemastore.org/api/json/catalog.json" in addition to built-in identifiers. This expanded functionality allows referencing external schema catalogs using the tombi:// scheme, which was not supported in earlier versions.
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`. 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: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (4)
ruff.toml (1)
7-23: Indentation clean-up looks goodOnly whitespace was adjusted; semantic content is unchanged.
tombi.toml (2)
1-6: Verify top-level key nameMost examples use
version = "1"(or similar) instead oftoml-version. Iftombidoesn’t recognisetoml-version, the file will be ignored and CI will fail. Please runtombi lintlocally to confirm the key is accepted.
11-14: Rule set aligns with previous guidance
key-emptyis the correct identifier; good catch.lint-tasks.yaml (1)
159-170: Nice addition of TOML lint & format tasksThe split between
toml-lintandtoml-formatmirrors the Python flow and keeps things consistent.
sitaowang1998
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
lint-tasks.yaml (2)
160-170: ❗ Unresolved merge-conflict markers make the Taskfile invalid YAML
<<<<<<< HEAD … ======= … >>>>>>> mainblocks are still present.
Their presence:
- Breaks YAML parsing (confirmed by yamllint error and CI failure).
- Causes the
toml-check/toml-formattasks to reference bothFLAGSandTOMBI_FORMAT_FLAGS, leading to ambiguous variable names.Minimal, conflict-free resolution:
toml-check: cmds: - task: "toml-lint" - task: "toml-format" -<<<<<<< HEAD - vars: - TOMBI_FORMAT_FLAGS: "--check" -======= - vars: - FLAGS: "--check" ->>>>>>> main + vars: + TOMBI_FORMAT_FLAGS: "--check" … toml-format: internal: true deps: ["venv"] -<<<<<<< HEAD - vars: - TOMBI_FORMAT_FLAGS: "{{.TOMBI_FORMAT_FLAGS | default \"\"}}" - cmds: - - |- - . "{{.G_LINT_VENV_DIR}}/bin/activate" - tombi format {{.TOMBI_FORMAT_FLAGS}} "{{.ROOT_DIR}}" -======= - vars: - FLAGS: "{{.FLAGS | default \"\"}}" - cmds: - - |- - . "{{.G_LINT_VENV_DIR}}/bin/activate" - tombi format {{.FLAGS}} "{{.ROOT_DIR}}" ->>>>>>> main + vars: + TOMBI_FORMAT_FLAGS: "{{.TOMBI_FORMAT_FLAGS | default \"\"}}" + cmds: + - |- + . "{{.G_LINT_VENV_DIR}}/bin/activate" + tombi format {{.TOMBI_FORMAT_FLAGS}} "{{.ROOT_DIR}}"Resolve the conflict and push a clean file; otherwise every downstream task will fail at runtime.
Also applies to: 188-200
132-139: Python lint scope limited totests/integrationonlyThe
forloop iterates solely over thetests/integrationdirectory. Source files (e.g.,src/,tools/, root scripts) escape MyPy and Ruff completely, undermining the new lint step.Consider parameterising the iterable:
- - for: - - "tests/integration" + - for: + - "src" + - "tests" + - "tools" + - "taskfile.yaml"or introduce a glob pattern to cover all
.pyfiles.Without widening the scope, the added
py-check/py-fixtasks provide little value.
🧹 Nitpick comments (1)
lint-tasks.yaml (1)
119-124:py-fixtriggers formatting but not format fixesInside
pythe sequence is:ruff check {{.RUFF_CHECK_FLAGS}} … ruff format {{.RUFF_FORMAT_FLAGS}} …
py-fixpasses only--fixtoRUFF_CHECK_FLAGS, soruff formatstill runs but with no flags, meaning it shows diff-less output rather than applying changes. If the intention is to auto-apply formatting as well, add--unsafe-fixes(or-no-flag) toRUFF_FORMAT_FLAGS.- - task: "py" - vars: - RUFF_CHECK_FLAGS: "--fix" + - task: "py" + vars: + RUFF_CHECK_FLAGS: "--fix" + RUFF_FORMAT_FLAGS: "--unsafe-fixes"Optional but prevents half-fixed code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lint-tasks.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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: 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: 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:
lint-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:
lint-tasks.yaml
🪛 YAMLlint (1.37.1)
lint-tasks.yaml
[error] 166-166: syntax error: could not find expected ':'
(syntax)
⏰ 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: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (1)
lint-tasks.yaml (1)
19-22: 👍py-fixcorrectly added to the global fix pipeline
fix.cmdsnow triggers automatic Python fixes alongside CMake and C++. Nice catch; this keeps parity with the newpy-fixtask.
33c876c to
e4c8dc5
Compare
There was a problem hiding this comment.
A few things to change:
some violations->any violations- We should sort tasks in the lexical order:
tomlcomes afterpy. cpp-static-fixandyml-fixdon't actually fix anything. We should just drop "fixes xxx".
There was a problem hiding this comment.
As discussed offline, let's use some violations when not all violations can be fixed, and use any violations otherwise. That said, we should change cpp-format-fix to use any violations.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
fix: Add `lint:py-fix` as a sub-task of `lint:fix`; Update README to include all lint tasks.
lint:py-fix as a sub-task of lint:fix; Update README to include all lint tasks.
2 participants
Description
Note
This PR depends on #176.
As title.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Documentation
Chores