Feature/Separate system- and user-settings#15
Open
barseghyanartur wants to merge 11 commits intodevfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a system vs user split for ignore patterns and file extensions with additive user-layer options, adds merge helpers to resolve configs, updates generate/resolve flows and CLI, bumps version to 0.2.5, expands tests for merging and per-file overrides, and adjusts docs, Makefile, and packaging metadata. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant ConfigResolver
participant MergeHelpers
participant Generator
User->>CLI: provide CLI flags or pyproject.toml
CLI->>ConfigResolver: parse config + flags
ConfigResolver->>MergeHelpers: _merge_ignore(cfg)
MergeHelpers->>MergeHelpers: combine SYSTEM_IGNORE + user_ignore
MergeHelpers-->>ConfigResolver: effective_ignore
ConfigResolver->>MergeHelpers: _merge_extensions(cfg)
MergeHelpers->>MergeHelpers: combine SYSTEM_EXTENSIONS + user_extensions
MergeHelpers-->>ConfigResolver: effective_extensions
ConfigResolver-->>Generator: resolved config (merged lists)
Generator->>Generator: collect_files(filtered by effective_ignore/extensions)
Generator->>Generator: apply per-file overrides (if any)
Generator-->>User: produced documentation/source-tree output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test_sphinx_source_tree.py (1)
30-32:⚠️ Potential issue | 🟡 MinorDuplicate entries in
__all__.
"TestLoadConfig"is repeated three times. This appears to be a copy-paste error.🐛 Proposed fix
"TestFileOptions", "TestGenerate", "TestLoadConfig", - "TestLoadConfig", - "TestLoadConfig", + "TestIgnoreExtensionsMerge", "TestMain", "TestOrder", "TestResolveConfig",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_sphinx_source_tree.py` around lines 30 - 32, The __all__ list contains duplicate string entries "TestLoadConfig"; remove the redundant occurrences so that "TestLoadConfig" appears only once in the __all__ export list (locate the __all__ definition and de-duplicate it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test_sphinx_source_tree.py`:
- Around line 1772-1789: The test test_generate_respects_user_ignore uses a
too-broad assertion ("app.py" not in rst) that can match "test_app.py"; update
the assertion to check the specific file path that should be excluded after
merging ignores (use the _merge_ignore result and generate call already present)
— for example assert that the relative path "src/app.py" (or str(Path("src") /
"app.py") if you prefer pathlib) is not in rst so only the intended excluded
file is checked.
---
Outside diff comments:
In `@test_sphinx_source_tree.py`:
- Around line 30-32: The __all__ list contains duplicate string entries
"TestLoadConfig"; remove the redundant occurrences so that "TestLoadConfig"
appears only once in the __all__ export list (locate the __all__ definition and
de-duplicate it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2756e373-0900-4254-a46c-0bf6ba488d06
📒 Files selected for processing (3)
README.rstsphinx_source_tree.pytest_sphinx_source_tree.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test_sphinx_source_tree.py (1)
1922-1939:⚠️ Potential issue | 🔴 CriticalNarrow the path assertion in
test_generate_respects_user_ignore.Line 1939 uses a broad substring check (
"app.py"), which can matchtests/test_app.pyand cause false failures.🐛 Proposed fix
def test_generate_respects_user_ignore(self, sample_project): """user_ignore patterns are actually excluded from output.""" from sphinx_source_tree import _merge_ignore merged = _merge_ignore( { "system_ignore": list(SYSTEM_IGNORE), "user_ignore": ["src"], "ignore": None, } ) rst = generate( project_root=sample_project, output=sample_project / "docs" / "out.rst", extensions=[".py"], ignore=merged, ) - assert "app.py" not in rst + assert "src/app.py" not in rst + assert "tests/test_app.py" in rst🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_sphinx_source_tree.py` around lines 1922 - 1939, The test test_generate_respects_user_ignore uses a broad substring assertion ("app.py") which can match files like tests/test_app.py; update the assertion to check the exact relative/path entry that should be excluded (e.g., assert the generated rst does not contain "src/app.py" or the full path segment produced by generate) or use a path-aware check (compare against lines or use a regex anchored to the path) so only the intended src/app.py is rejected; locate test_generate_respects_user_ignore and the generate(...) call to adjust the assertion accordingly.
🧹 Nitpick comments (1)
sphinx_source_tree.py (1)
327-351: Consider extracting the per-file config re-merging logic into a dedicated helper function.The conditional logic for re-resolving
ignoreandextensionsat lines 327–351 is repetitive and difficult to follow. While the logic itself is correct, extracting this into a helper function with clear documentation would improve maintainability. The function should handle each scenario explicitly:
- Entry with explicit
ignore(use as-is)- Entry with
user_ignoreorsystem_ignoreonly (re-merge with top-level system defaults)- Entry with neither (use top-level resolved value)
This refactoring would also make it easier to write unit tests for each resolution scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sphinx_source_tree.py` around lines 327 - 351, Extract the repeated re-resolving logic into a helper (e.g., resolve_per_file_config(file_resolved, entry)) that takes the per-file entry and its current resolved dict and performs the three explicit cases for both ignore and extensions: if entry contains explicit "ignore"/"extensions" return it as-is; elif entry contains "user_ignore"/"system_ignore" or "user_extensions"/"system_extensions" clear the top-level precomputed value (set file_resolved["ignore"] or ["extensions"] = None) and call _merge_ignore(file_resolved) or _merge_extensions(file_resolved); else leave the top-level resolved value untouched. Replace the duplicated blocks around file_resolved, entry, _merge_ignore and _merge_extensions with a single call to this helper and add a short docstring describing the three resolution scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sphinx_source_tree.py`:
- Around line 165-178: The code declares VALID_FILE_OPTIONS including
"description" and "url" but the literalinclude generator does not skip those in
full mode; update the literalinclude-building logic to filter out "description"
and "url" when mode == "full" (i.e., ensure the same filtering applied to other
options uses VALID_FILE_OPTIONS but excludes description and url for full-mode
literalinclude generation), modifying the literalinclude generation path (the
code that currently handles option filtering where VALID_FILE_OPTIONS is
referenced) so that description and url are omitted from the emitted
literalinclude options in full mode.
- Around line 919-920: The loop emitting literalinclude options currently
appends every validated option from _file_options[rel], but must skip
headers-only options "description" and "url" when in full (non-headers) mode;
update the loop that iterates over for opt_key, opt_val in
_file_options.get(rel, {}).items() (the code that calls block.append(f"
:{opt_key}: {opt_val}")) to filter keys against a whitelist (e.g.,
LITERALINCLUDE_OPTIONS) or explicitly skip opt_key in {"description","url"} when
not rendering headers so only valid options are added to the literalinclude
block.
---
Duplicate comments:
In `@test_sphinx_source_tree.py`:
- Around line 1922-1939: The test test_generate_respects_user_ignore uses a
broad substring assertion ("app.py") which can match files like
tests/test_app.py; update the assertion to check the exact relative/path entry
that should be excluded (e.g., assert the generated rst does not contain
"src/app.py" or the full path segment produced by generate) or use a path-aware
check (compare against lines or use a regex anchored to the path) so only the
intended src/app.py is rejected; locate test_generate_respects_user_ignore and
the generate(...) call to adjust the assertion accordingly.
---
Nitpick comments:
In `@sphinx_source_tree.py`:
- Around line 327-351: Extract the repeated re-resolving logic into a helper
(e.g., resolve_per_file_config(file_resolved, entry)) that takes the per-file
entry and its current resolved dict and performs the three explicit cases for
both ignore and extensions: if entry contains explicit "ignore"/"extensions"
return it as-is; elif entry contains "user_ignore"/"system_ignore" or
"user_extensions"/"system_extensions" clear the top-level precomputed value (set
file_resolved["ignore"] or ["extensions"] = None) and call
_merge_ignore(file_resolved) or _merge_extensions(file_resolved); else leave the
top-level resolved value untouched. Replace the duplicated blocks around
file_resolved, entry, _merge_ignore and _merge_extensions with a single call to
this helper and add a short docstring describing the three resolution scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3e395a2-8725-4c89-b695-a9851598a1ac
📒 Files selected for processing (15)
.gitignore.secrets.baselineCHANGELOG.rstCONTRIBUTING.rstMakefileREADME.rstdocs/CONTRIBUTING.rstdocs/README.rstdocs/documentation.rstdocs/full-llms.rstdocs/llms.rstdocs/source_tree.rstpyproject.tomlsphinx_source_tree.pytest_sphinx_source_tree.py
💤 Files with no reviewable changes (1)
- docs/source_tree.rst
✅ Files skipped from review due to trivial changes (3)
- docs/CONTRIBUTING.rst
- docs/README.rst
- CHANGELOG.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- README.rst
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
AGENTS.md (2)
140-140: Consider documenting the Makefile or clarifying the pre-commit command.Line 140 references
make pre-commit, but the Makefile is not listed in the "Key files" section (lines 99-108). Consider either adding the Makefile to the key files table with its available targets, or replacing this with an explicit command likeruff check . && isort --check .for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 140, Update AGENTS.md to either add the Makefile to the "Key files" table (listing its available targets including pre-commit) or replace the ambiguous `make pre-commit` reference with the explicit command(s) it runs (e.g., `ruff check . && isort --check .`) so readers know exactly what to run; locate the `make pre-commit` mention and the "Key files" section and modify the text accordingly to include the Makefile entry or the explicit command string.
111-114: Add language specifiers to fenced code blocks.Three code blocks are missing language specifiers, which reduces readability in Markdown renderers and prevents syntax highlighting:
- Lines 111-114: Public API symbols (could use
textorpython)- Lines 118-120: Config resolution order (could use
text)- Lines 170-192: RST heading hierarchy (could use
rstortext)📝 Proposed fix to add language specifiers
For lines 111-114:
-``` +```text build_parser build_tree collect_files detect_language generate load_config main resolve_configFor lines 118-120: ```diff -``` +```text DEFAULTS < [tool.sphinx-source-tree] < [[…files]] entry < CLI argsFor lines 170-192: ```diff -``` +```rst ===== titleAlso applies to: 118-120, 170-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 111 - 114, The Markdown has three fenced code blocks missing language specifiers; update the blocks that show the public API symbols (the block containing "build_parser build_tree collect_files detect_language generate load_config main resolve_config"), the config resolution order block (the block containing "DEFAULTS < [tool.sphinx-source-tree] < [[…files]] entry < CLI args"), and the RST heading hierarchy block (the block with the RST heading markers and "title") by adding appropriate language tags (e.g., use "text" or "python" for the public API symbols block, "text" for the config order block, and "rst" or "text" for the RST headings) so Markdown renderers can apply syntax highlighting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 20: The doc contains conflicting Python version statements: the phrase
"Zero required dependencies for Python 3.11+; only `tomli` for 3.10." and the
line "Target: `py39` (ruff), `Python 3.10+` (runtime)" should be unified; update
AGENTS.md to state a single minimum runtime (e.g., "Python 3.10+") and mention
conditional dependency behavior ("no extra deps on 3.11+, `tomli` required for
3.10") and change the ruff target from `py39` to the matching target (e.g.,
`py310`) so the "Zero required dependencies..." sentence and the "Target:
`py39`..." sentence consistently reflect the same minimum Python version.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 140: Update AGENTS.md to either add the Makefile to the "Key files" table
(listing its available targets including pre-commit) or replace the ambiguous
`make pre-commit` reference with the explicit command(s) it runs (e.g., `ruff
check . && isort --check .`) so readers know exactly what to run; locate the
`make pre-commit` mention and the "Key files" section and modify the text
accordingly to include the Makefile entry or the explicit command string.
- Around line 111-114: The Markdown has three fenced code blocks missing
language specifiers; update the blocks that show the public API symbols (the
block containing "build_parser build_tree collect_files detect_language generate
load_config main resolve_config"), the config resolution order block (the block
containing "DEFAULTS < [tool.sphinx-source-tree] < [[…files]] entry < CLI
args"), and the RST heading hierarchy block (the block with the RST heading
markers and "title") by adding appropriate language tags (e.g., use "text" or
"python" for the public API symbols block, "text" for the config order block,
and "rst" or "text" for the RST headings) so Markdown renderers can apply syntax
highlighting.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test_sphinx_source_tree.py (1)
1789-1789:⚠️ Potential issue | 🟡 MinorAssert on the concrete excluded path instead of the substring.
assert "app.py" not in rststill fails iftests/test_app.pyremains in the output, so this test can report a false negative even whensrc/app.pyis correctly excluded.🐛 Proposed fix
- assert "app.py" not in rst + assert "src/app.py" not in rst + assert "tests/test_app.py" in rst🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_sphinx_source_tree.py` at line 1789, The test currently asserts the substring "app.py" is not in rst which can produce false negatives; update the assertion to check the concrete excluded path string that should be absent (e.g., "src/app.py" or the exact path/filename format used in rst) — replace assert "app.py" not in rst with an assertion against the full excluded path variable or literal (for example assert "src/app.py" not in rst) so the test only fails when the exact excluded file is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Line 220: The ignore pattern "docs/examples" only matches the directory itself
but not its descendants; update how ignore patterns are generated/normalized in
sphinx_source_tree.py (the code that builds the matcher used by collect_files())
to also add a descendant glob (e.g. append "/**" or equivalent) for patterns
that target directories (like "docs/examples" and "docs/_build") so the matcher
treats them as subtree exclusions; modify the logic that constructs the matcher
in the SphinxSourceTree/collect_files flow to emit both the directory pattern
and a corresponding descendant glob so files under those subtrees are excluded.
In `@sphinx_source_tree.py`:
- Around line 310-335: The re-resolution logic is inadvertently discarding
explicit CLI-applied replacements because it checks for "ignore"/"extensions" in
entry rather than whether a top-level/CLI value is already present in
file_resolved; update the branches around _merge_ignore and _merge_extensions so
you only clear file_resolved["ignore"] / file_resolved["extensions"] and call
_merge_* when there is no explicit top-level/CLI value present (i.e., check
"ignore" / "extensions" in file_resolved, not just in entry) and user/system
parts require re-resolve; keep the existing checks for
"user_ignore"/"system_ignore" and "user_extensions"/"system_extensions" but
guard the clearing/recompute on absence of the explicit value in file_resolved
to preserve CLI wins.
---
Duplicate comments:
In `@test_sphinx_source_tree.py`:
- Line 1789: The test currently asserts the substring "app.py" is not in rst
which can produce false negatives; update the assertion to check the concrete
excluded path string that should be absent (e.g., "src/app.py" or the exact
path/filename format used in rst) — replace assert "app.py" not in rst with an
assertion against the full excluded path variable or literal (for example assert
"src/app.py" not in rst) so the test only fails when the exact excluded file is
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe8fd091-d327-40e5-bb32-e1aa3769beb8
📒 Files selected for processing (5)
README.rstdocs/documentation.rstpyproject.tomlsphinx_source_tree.pytest_sphinx_source_tree.py
Comment on lines
+310
to
+335
| if needs_rematch: | ||
| # Reset to None so _merge_* uses the per-file parts | ||
| if ( | ||
| "ignore" not in entry | ||
| and "user_ignore" not in entry | ||
| and "system_ignore" not in entry | ||
| ): | ||
| pass # keep top-level resolved ignore | ||
| else: | ||
| # Re-resolve: clear the pre-computed value so _merge_* | ||
| # uses the (possibly updated) system/user parts. | ||
| if "ignore" not in entry: | ||
| file_resolved["ignore"] = None | ||
| file_resolved["ignore"] = _merge_ignore(file_resolved) | ||
|
|
||
| if ( | ||
| "extensions" not in entry | ||
| and "user_extensions" not in entry | ||
| and "system_extensions" not in entry | ||
| ): | ||
| pass | ||
| else: | ||
| if "extensions" not in entry: | ||
| file_resolved["extensions"] = None | ||
| file_resolved["extensions"] = _merge_extensions( | ||
| file_resolved |
There was a problem hiding this comment.
Do not discard explicit CLI replacements during per-file re-resolution.
If a [[files]] entry only sets user_ignore / user_extensions, this branch clears the already-applied CLI ignore / extensions value and recomputes from the layered lists. A command like --ignore foo therefore stops winning for that output as soon as the file entry adds user-ignore, which breaks the documented precedence rule.
🐛 Proposed fix
if needs_rematch:
+ ignore_explicit = (
+ "ignore" in entry or "ignore" in cli_overrides
+ )
+ extensions_explicit = (
+ "extensions" in entry
+ or "extensions" in cli_overrides
+ )
+
- if (
- "ignore" not in entry
- and "user_ignore" not in entry
- and "system_ignore" not in entry
- ):
- pass # keep top-level resolved ignore
- else:
- # Re-resolve: clear the pre-computed value so _merge_*
- # uses the (possibly updated) system/user parts.
- if "ignore" not in entry:
- file_resolved["ignore"] = None
- file_resolved["ignore"] = _merge_ignore(file_resolved)
+ if not ignore_explicit and (
+ "user_ignore" in entry or "system_ignore" in entry
+ ):
+ file_resolved["ignore"] = _merge_ignore(
+ {**file_resolved, "ignore": None}
+ )
- if (
- "extensions" not in entry
- and "user_extensions" not in entry
- and "system_extensions" not in entry
- ):
- pass
- else:
- if "extensions" not in entry:
- file_resolved["extensions"] = None
- file_resolved["extensions"] = _merge_extensions(
- file_resolved
- )
+ if not extensions_explicit and (
+ "user_extensions" in entry
+ or "system_extensions" in entry
+ ):
+ file_resolved["extensions"] = _merge_extensions(
+ {**file_resolved, "extensions": None}
+ )A small regression test around --ignore / --extensions plus [[files]].user-* would lock this down.
📝 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.
Suggested change
| if needs_rematch: | |
| # Reset to None so _merge_* uses the per-file parts | |
| if ( | |
| "ignore" not in entry | |
| and "user_ignore" not in entry | |
| and "system_ignore" not in entry | |
| ): | |
| pass # keep top-level resolved ignore | |
| else: | |
| # Re-resolve: clear the pre-computed value so _merge_* | |
| # uses the (possibly updated) system/user parts. | |
| if "ignore" not in entry: | |
| file_resolved["ignore"] = None | |
| file_resolved["ignore"] = _merge_ignore(file_resolved) | |
| if ( | |
| "extensions" not in entry | |
| and "user_extensions" not in entry | |
| and "system_extensions" not in entry | |
| ): | |
| pass | |
| else: | |
| if "extensions" not in entry: | |
| file_resolved["extensions"] = None | |
| file_resolved["extensions"] = _merge_extensions( | |
| file_resolved | |
| if needs_rematch: | |
| ignore_explicit = ( | |
| "ignore" in entry or "ignore" in cli_overrides | |
| ) | |
| extensions_explicit = ( | |
| "extensions" in entry | |
| or "extensions" in cli_overrides | |
| ) | |
| if not ignore_explicit and ( | |
| "user_ignore" in entry or "system_ignore" in entry | |
| ): | |
| file_resolved["ignore"] = _merge_ignore( | |
| {**file_resolved, "ignore": None} | |
| ) | |
| if not extensions_explicit and ( | |
| "user_extensions" in entry | |
| or "system_extensions" in entry | |
| ): | |
| file_resolved["extensions"] = _merge_extensions( | |
| {**file_resolved, "extensions": None} | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sphinx_source_tree.py` around lines 310 - 335, The re-resolution logic is
inadvertently discarding explicit CLI-applied replacements because it checks for
"ignore"/"extensions" in entry rather than whether a top-level/CLI value is
already present in file_resolved; update the branches around _merge_ignore and
_merge_extensions so you only clear file_resolved["ignore"] /
file_resolved["extensions"] and call _merge_* when there is no explicit
top-level/CLI value present (i.e., check "ignore" / "extensions" in
file_resolved, not just in entry) and user/system parts require re-resolve; keep
the existing checks for "user_ignore"/"system_ignore" and
"user_extensions"/"system_extensions" but guard the clearing/recompute on
absence of the explicit value in file_resolved to preserve CLI wins.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test_sphinx_source_tree.py (1)
24-36:⚠️ Potential issue | 🟡 MinorDuplicate entries in
__all__.The
__all__tuple contains"TestLoadConfig"three times (lines 31-33). This doesn't cause runtime errors but indicates a copy-paste oversight. Consider replacing the duplicates with the actual test class names.🐛 Proposed fix
__all__ = ( "TestBuildTree", "TestCollectFiles", "TestDetectLanguage", "TestFileOptions", "TestGenerate", - "TestLoadConfig", - "TestLoadConfig", - "TestLoadConfig", + "TestIgnoreExtensionsMerge", + "TestLoadConfig", + "TestResolveFileOptionsProfile", "TestMain", "TestOrder", "TestResolveConfig", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_sphinx_source_tree.py` around lines 24 - 36, The __all__ tuple contains three duplicate "TestLoadConfig" entries; update the __all__ definition (symbol __all__) to remove duplicates and replace them with the correct test class names present in the module (inspect the file for class definitions like TestBuildTree, TestCollectFiles, TestDetectLanguage, TestFileOptions, TestGenerate, TestLoadConfig, TestMain, TestOrder, TestResolveConfig) so each exported test class appears exactly once.
🧹 Nitpick comments (2)
Makefile (2)
68-78: Minor: Duplicate cleanup ofdist/directory.Lines 68 and 78 both execute
rm -rf dist/. This is redundant.♻️ Proposed fix
rm -rf build/ rm -rf dist/ rm -rf .cache/ rm -rf htmlcov/ rm -rf builddocs/ rm -rf testdocs/ rm -rf .coverage rm -rf .pytest_cache/ rm -rf .mypy_cache/ rm -rf .ruff_cache/ - rm -rf dist/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 68 - 78, Remove the duplicate removal of the dist/ directory in the Makefile cleanup section: locate the two identical commands "rm -rf dist/" in the block that contains rm -rf build/, rm -rf .cache/, etc., and delete one of them so dist/ is only cleaned once.
41-46: LGTM with a minor note.The rename from
venvtocreate-venvis more descriptive. Theuv synccommands are correct.Note: The
VENVvariable (line 5) appears to be unused after the migration touv run. Consider removing it in a future cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 41 - 46, The Makefile still defines the VENV variable but it is no longer used after switching to uv run; remove the unused VENV variable declaration to clean up the file and avoid confusion, and verify only the targets create-venv and install (which call "uv sync" and "uv sync --all-extras") remain referencing uv so there are no leftover references to VENV elsewhere in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.rst`:
- Around line 25-35: Remove or defer the changelog claim about a new "mode"
option because it is not implemented: delete or revert the paragraph in the
0.2.4 entry that describes the ``mode`` option (``full`` | ``headers``), the
``--mode`` CLI flag, and the ``mode`` keyword argument to generate(), so the
CHANGELOG no longer documents non-existent features; alternatively, if you
prefer to keep a note, change the text to explicitly mark the feature as
"planned" or "deferred" rather than implemented.
In `@Makefile`:
- Around line 80-84: Update the Makefile targets compile-requirements and
compile-requirements-upgrade to use the correct uv CLI form: replace the invalid
"uv run pip compile" invocations with "uv pip compile" so both targets call "uv
pip compile --all-extras -o docs/requirements.txt pyproject.toml" (and add the
--upgrade flag for compile-requirements-upgrade); modify the command strings for
the targets named compile-requirements and compile-requirements-upgrade
accordingly.
---
Outside diff comments:
In `@test_sphinx_source_tree.py`:
- Around line 24-36: The __all__ tuple contains three duplicate "TestLoadConfig"
entries; update the __all__ definition (symbol __all__) to remove duplicates and
replace them with the correct test class names present in the module (inspect
the file for class definitions like TestBuildTree, TestCollectFiles,
TestDetectLanguage, TestFileOptions, TestGenerate, TestLoadConfig, TestMain,
TestOrder, TestResolveConfig) so each exported test class appears exactly once.
---
Nitpick comments:
In `@Makefile`:
- Around line 68-78: Remove the duplicate removal of the dist/ directory in the
Makefile cleanup section: locate the two identical commands "rm -rf dist/" in
the block that contains rm -rf build/, rm -rf .cache/, etc., and delete one of
them so dist/ is only cleaned once.
- Around line 41-46: The Makefile still defines the VENV variable but it is no
longer used after switching to uv run; remove the unused VENV variable
declaration to clean up the file and avoid confusion, and verify only the
targets create-venv and install (which call "uv sync" and "uv sync
--all-extras") remain referencing uv so there are no leftover references to VENV
elsewhere in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f9ceaa4-308d-4793-a4d1-32ddabbea646
📒 Files selected for processing (6)
CHANGELOG.rstMakefileREADME.rstpyproject.tomlsphinx_source_tree.pytest_sphinx_source_tree.py
barseghyanartur
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores