chore(lint): Update Ruff config to match yscope-dev-utils and resolve resulting lint issues. - #173
Conversation
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
sitaowang1998
changed the title
yscope-dev-utils.yscope-dev-utils.
![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 (4)
lint-tasks.yaml (4)
62-65: Reuse thecpp_source_filesanchor in other tasks to avoid silent drift
&cpp_source_filesis defined here but later the same list is repeated verbatim (Lines 96-97 and 107-108). Referencing the anchor would make future path changes single-point-edits.- ROOT_PATHS: - - "{{.G_SRC_SPIDER_DIR}}" - - "{{.G_TEST_DIR}}" + ROOT_PATHS: *cpp_source_filesSame for Lines 107-108.
85-98: Anchor can replace duplicated ROOT_PATHS incpp-static-checkAs noted above, duplicate path literals appear twice inside the
cpp-static-checktask. Swapping them for*cpp_source_fileskeeps the task definition short and guarantees both format and static analysis cover identical trees.
114-121: Clarify ruff flags for check vs. fix runsThe current pairing works, but:
py-checkprints full diffs (--diff), which may add noisy logs;--checkis quieter and still fails CI on style violations.py-fixrunsruff formatwith no flags, which rewrites files in place. If accidental commits of formatted files are a concern, consider--unsafe-fixesor keeping the two-step “check → fix” symmetry.No action required, just something to weigh.
124-134: Minor: avoid repeated venv activation inside very short loopThe loop currently activates the virtual-env once per ITEM (only one element today). If the list ever grows, wrapping the loop body in a function or hoisting activation outside will save a few process spawns.
Purely optional.
📜 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 (2)
📓 Common learnings
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.
📚 Learning: antlr-generated c++ code in the g_src_dsl_dir (src/stdl) should not be included in linting tasks bec...
Learnt from: sitaowang1998
PR: y-scope/spider#168
File: lint-tasks.yaml:62-65
Timestamp: 2025-07-17T19:44:06.132Z
Learning: ANTLR-generated C++ code in the G_SRC_DSL_DIR (src/stdl) should not be included in linting tasks because it's auto-generated code that doesn't follow manual coding standards.
Applied to files:
lint-tasks.yaml
🔇 Additional comments (1)
lint-tasks.yaml (1)
48-58: Nice use of YAML anchors to de-duplicate the C++ source listIntroducing the
&cpp_lint_source_filesanchor and re-using it elsewhere keeps the three C++ lint tasks in sync and eliminates a maintenance hotspot.
👍 No issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lint-tasks.yaml (2)
110-116: Order of operations may suppress theruff format --diffoutput
ruff checkwill exit with a non-zero status when violations are detected. Because the two commands are in the same shell block, theruff format --diffline on Line 134 will never run onceruff checkfails, hiding the formatting diff that the task is meant to surface.
Consider running the format pass first or guarding the second command, e.g.:- ruff check {{.RUFF_CHECK_FLAGS}} "{{.ITEM}}" - ruff format {{.RUFF_FORMAT_FLAGS}} "{{.ITEM}}" + ruff format {{.RUFF_FORMAT_FLAGS}} "{{.ITEM}}" + ruff check {{.RUFF_CHECK_FLAGS}} "{{.ITEM}}" || trueThis preserves the diff output while still letting the task fail overall on lint errors.
124-127: Minor template quoting nitThe Go-template
defaultfunction already returns an empty string when the variable is undefined, so escaping the inner quotes is unnecessary noise. Using single quotes makes the expression easier to read:- RUFF_CHECK_FLAGS: "{{.RUFF_CHECK_FLAGS | default \"\"}}" - RUFF_FORMAT_FLAGS: "{{.RUFF_FORMAT_FLAGS | default \"\"}}" + RUFF_CHECK_FLAGS: '{{ .RUFF_CHECK_FLAGS | default "" }}' + RUFF_FORMAT_FLAGS: '{{ .RUFF_FORMAT_FLAGS | default "" }}'Purely cosmetic, but it improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lint-requirements.txt(0 hunks)lint-tasks.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- lint-requirements.txt
⏰ 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)
114-120: Verify that both flag sets are intentionally omitted/passed
py-checksets onlyRUFF_FORMAT_FLAGS, whilepy-fixsets onlyRUFF_CHECK_FLAGS. This means:•
py-checklints (no flags) and shows a diff, but never formats.
•py-fixfixes lint errors and formats in-place, but never prints a diff.If that split is deliberate—great. If the intent was to use both flag groups in each task, update the variable blocks accordingly.
There was a problem hiding this comment.
Directly pushed some changes to this file to polish the docstring. What we should be aware of for the future docstrings to be consistent with our Python docstring guidelines:
- For the docstring of a class, we just start with "Represents ...". Don't repeat the class name.
- When u need to refer to a class name, a param name, or a variable name, use "`" to surround the symbols.
- Docstring for functions should usually start with a verb in the singular form.
- For parameters' docstring, capitalize the first char (and don't forget "the").
- For return values' docstring, also capitalize the first char.
- Don't document the return values if the function returns None.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/test_client.py (1)
80-82: Fix docstring formatting.The docstring should be formatted as a one-liner according to the Ruff warning.
- """ - Executes the `client_test` program and checks for successful execution. - """ + """Executes the `client_test` program and checks for successful execution."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/integration/test_client.py(3 hunks)tests/integration/test_scheduler_worker.py(10 hunks)tests/integration/test_signal.py(5 hunks)tests/integration/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/utils.py
- tests/integration/test_signal.py
- tests/integration/test_scheduler_worker.py
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/integration/test_client.py
80-82: One-line docstring should fit on one line
Reformat to one line
(D200)
⏰ 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 (7)
tests/integration/test_client.py (7)
1-1: LGTM! Clear module documentation.The module docstring provides a clear description of the file's purpose.
5-5: LGTM! Appropriate imports for type annotations.The new imports support the enhanced type annotations used throughout the file. Using
collections.abc.Generatoris the modern approach preferred overtyping.Generator.Also applies to: 8-8
19-25: LGTM! Improved type annotations and documentation.The change from
Tupletotuplealigns with modern Python best practices for built-in types. The docstring is comprehensive and well-formatted, clearly documenting parameters and return values.
54-63: LGTM! Well-typed fixture with proper documentation.The type annotations accurately reflect the fixture's interface. The docstring clearly explains the fixture's lifecycle and purpose. The explicit handling of the unused
storageparameter with the ARG001 comment is a good practice for silencing linter warnings while maintaining dependency injection.
76-76: LGTM! Clear class documentation.The class docstring appropriately describes the test class purpose.
78-79: LGTM! Cleaner test method signature.Using
@pytest.mark.usefixturesinstead of accepting the fixture as a parameter results in a cleaner method signature while maintaining the fixture dependency.
90-91: LGTM! Improved error handling.Adding
check=Trueto the subprocess.run call is an excellent improvement. It will raise a CalledProcessError if the process fails, making test failures more explicit and informative compared to just checking the return code.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Directly pushed to polish the docstrings.
|
|
||
| def _get_free_tcp_port() -> int: | ||
| """Returns a free TCP port.""" | ||
| """:return: A free TCP port number.""" |
There was a problem hiding this comment.
The description is repetitive. In this case, keeping only the return should be fine.
| :return: A tuple of the started processes: | ||
| - The scheduler process. | ||
| - The first worker process. | ||
| - The second worker process. |
There was a problem hiding this comment.
This is usually how we document tuples/pairs.
| :param storage: | ||
| :param success_job: |
There was a problem hiding this comment.
For these test parameters, you can just leave them empty.
The removed description isn't 100% correct: the parameter is a generator, not an object.
There was a problem hiding this comment.
These type annotations are actually wrong and are fixed in #174.
There was a problem hiding this comment.
lol, alright. Let's fix the type annotation in #174.
Are u ok to leave the parameter docstrings empty?
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
chore(lint): Update Ruff config to match `yscope-dev-utils` and resolve resulting lint issues.
yscope-dev-utils.yscope-dev-utils and resolve resulting lint issues.
2 participants
Description
Note
This PR depends on #166.
This PR:
yscope-dev-utilsto include new python linting configs.ruff.tomlfromyscope-dev-utils.ruff.tomlyscope-dev-utils.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Documentation
Style
Chores