Tighten screws by making all checks harder + fix any issues#27
Merged
Tighten screws by making all checks harder + fix any issues#27
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughPromotes several private helpers to public APIs, centralizes parsing/enrichment logic, modernizes I/O to Path.open and importlib.resources, adds publish validation/asset-minification, tightens recipe/path parsing safety, expands tooling/CI configs, and updates tests to use the new public names. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config as ConfigLoader
participant Publish as PublishCmd
participant Resources as importlib.resources
participant FS as Filesystem
CLI->>Config: load_config_with_data_dir()
Config-->>CLI: data_dir, config
CLI->>Publish: _cmd_publish(parsed)
Publish->>Publish: _validate_output_dir(data_dir, output)
Publish->>Resources: files("dfetch_hub") -> site assets
Resources-->>Publish: site_dir
Publish->>FS: remove/create output dir
Publish->>FS: _copy_assets(site_dir, output) (rewrite index.html paths)
Publish->>FS: _minify_catalog(data_dir, output/data) (read/write via Path.open)
Publish-->>CLI: log completed (count)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
dfetch_hub/catalog/sources/vcpkg.py (1)
99-99: Use idiomaticPathinstance method call.
Path.open(manifest_path, ...)calls the unbound method, which works but is unconventional. The standard pattern is to call the method on the instance directly.♻️ Proposed fix
- with Path.open(manifest_path, encoding="utf-8") as fh: + with manifest_path.open(encoding="utf-8") as fh:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch_hub/catalog/sources/vcpkg.py` at line 99, The call uses the unbound method Path.open(manifest_path, encoding="utf-8") which is unconventional; change it to call the instance method on manifest_path (i.e., manifest_path.open(encoding="utf-8")), ensuring manifest_path is a pathlib.Path instance before use (in the same scope where Path.open is currently invoked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 92-96: The fenced code block showing the shell commands (the
triple-backtick block containing "source .venv/bin/activate", "pre-commit run
--all-files", and "pytest") needs a language tag to satisfy markdownlint MD040;
update the opening fence from ``` to ```bash so the block is recognized as
Bash/sh syntax (i.e., change the existing ``` to ```bash for that code block).
In `@dfetch_hub/catalog/sources/readme.py`:
- Around line 24-25: The skip-regex _SKIP_RE is currently tested against the raw
line which allows indented headings/badges (e.g. " # Title" or " [![...]") to
slip through; update the logic that uses _SKIP_RE (the place where lines are
checked for descriptions around the README parsing) to normalize the line first
(e.g., call lstrip() to remove leading whitespace or otherwise normalize
indentation) before testing with _SKIP_RE so indented headings, badges, and
blank lines are correctly skipped by _SKIP_RE; keep every other behavior the
same.
In `@dfetch_hub/catalog/writer.py`:
- Line 86: In _save_json replace the incorrect use of the Path class constructor
with the Path instance's open method: change the call that uses Path.open(path,
"w", encoding="utf-8") to use path.open("w", encoding="utf-8") inside the
_save_json function so the file is opened on the provided Path object rather
than calling the class method incorrectly.
- Line 79: The call uses the class method syntax Path.open(path,
encoding="utf-8") which passes the Path object as the mode and raises TypeError;
change it to use the instance method by calling path.open(encoding="utf-8") so
the Path instance is the receiver (ensure the variable named path is a
pathlib.Path and remains unchanged in the surrounding context, e.g., inside the
writer function or method where this with-statement appears).
In `@dfetch_hub/config.py`:
- Around line 93-99: Update the docstring for the public function load_config in
dfetch_hub/config.py to document the new TypeError cases: add entries under the
Raises: section stating that TypeError is raised when the `[settings]` section
is not a TOML table (i.e., when data.get("settings") is not a dict) and when
`[[source]]` is not an array of TOML tables (i.e., when raw_sources_obj from
data.get("source", []) is not a list or any element is not a dict); reference
load_config and mention the specific conditions (non-dict settings and
non-list/invalid-list elements for raw_sources_obj) so callers know when
TypeError will be thrown.
---
Nitpick comments:
In `@dfetch_hub/catalog/sources/vcpkg.py`:
- Line 99: The call uses the unbound method Path.open(manifest_path,
encoding="utf-8") which is unconventional; change it to call the instance method
on manifest_path (i.e., manifest_path.open(encoding="utf-8")), ensuring
manifest_path is a pathlib.Path instance before use (in the same scope where
Path.open is currently invoked).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.pre-commit-config.yamlCLAUDE.mddfetch_hub/catalog/sources/__init__.pydfetch_hub/catalog/sources/clib.pydfetch_hub/catalog/sources/conan.pydfetch_hub/catalog/sources/readme.pydfetch_hub/catalog/sources/vcpkg.pydfetch_hub/catalog/writer.pydfetch_hub/commands/__init__.pydfetch_hub/commands/publish.pydfetch_hub/commands/serve.pydfetch_hub/commands/update.pydfetch_hub/config.pypyproject.tomltest/test_catalog_clib.pytest/test_catalog_sources.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch_hub/catalog/sources/__init__.py`:
- Around line 59-65: Add Google-style docstrings for the two public functions
raw_url and fetch_raw: replace the one-line docstrings with multi-line
docstrings that include Args: (document owner, repo, branch, filename for
raw_url; url for fetch_raw), Returns: (str for raw_url; str | None for
fetch_raw) and Raises: where relevant (e.g., document that fetch_raw may raise
network/requests exceptions or return None on failure). Ensure the docstrings
follow Google style and are placed immediately above the function definitions
for raw_url and fetch_raw.
In `@dfetch_hub/catalog/sources/clib.py`:
- Around line 123-125: The description assignment in _enrich_from_pkg_json
currently prefers the wiki tagline over package.json description; change it so
package.json description takes precedence: use pkg_json.get("description")
first, then fallback to tagline, then empty string, and ensure you still coerce
to str or None consistently with how package_name and license_val are handled
(refer to variables description, tagline, pkg_json and function
_enrich_from_pkg_json).
In `@dfetch_hub/commands/__init__.py`:
- Around line 45-46: Replace the noisy traceback log call
_logger.exception("Config file '%s' not found", config_path) with a
non-exception-level log (e.g., _logger.warning("Config file '%s' not found",
config_path) or _logger.debug(...)) in the same branch that exits via
sys.exit(1); this keeps the exit behavior (sys.exit(1)) but uses
_logger.warning/_logger.debug instead of _logger.exception to avoid emitting an
unnecessary traceback for an expected CLI/config condition.
In `@dfetch_hub/commands/publish.py`:
- Around line 31-34: The calls use the Path.open class method incorrectly;
change the two usages of Path.open(src, ...) and Path.open(dst, ...) to instance
calls on the Path objects (use src.open(... ) and dst.open(...)) so the instance
method on the Path variables src and dst is invoked; update the two with-blocks
in publish.py that currently call Path.open to use src.open and dst.open
respectively.
- Around line 107-110: The code currently calls shutil.rmtree(output) whenever
output.exists(), which will raise if output is a file or symlink; change the
flow to check output.is_dir() and only call shutil.rmtree(output) for
directories, otherwise call output.unlink() (which handles files and symlinks)
so logger.print_info_line("publish", ...) remains accurate and the subsequent
output.mkdir(parents=True) runs under your controlled error flow; update the
block around the output existence check (where shutil.rmtree is used) to perform
this conditional removal.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/test.yml.pre-commit-config.yamlCLAUDE.mddfetch_hub/catalog/cloner.pydfetch_hub/catalog/sources/__init__.pydfetch_hub/catalog/sources/clib.pydfetch_hub/catalog/sources/conan.pydfetch_hub/catalog/sources/readme.pydfetch_hub/catalog/sources/vcpkg.pydfetch_hub/catalog/writer.pydfetch_hub/cli.pydfetch_hub/commands/__init__.pydfetch_hub/commands/publish.pydfetch_hub/commands/serve.pydfetch_hub/commands/update.pydfetch_hub/config.pypyproject.tomltest/test_catalog_clib.pytest/test_catalog_cloner.pytest/test_catalog_conan.pytest/test_catalog_readme.pytest/test_catalog_sources.pytest/test_catalog_vcpkg.pytest/test_catalog_writer.py
✅ Files skipped from review due to trivial changes (6)
- test/test_catalog_conan.py
- test/test_catalog_cloner.py
- test/test_catalog_vcpkg.py
- test/test_catalog_writer.py
- test/test_catalog_readme.py
- dfetch_hub/catalog/cloner.py
🚧 Files skipped from review as they are similar to previous changes (6)
- test/test_catalog_sources.py
- dfetch_hub/config.py
- .pre-commit-config.yaml
- pyproject.toml
- test/test_catalog_clib.py
- dfetch_hub/catalog/sources/readme.py
Comment on lines
+59
to
65
| def raw_url(owner: str, repo: str, branch: str, filename: str) -> str: | ||
| """Build a raw.githubusercontent.com URL for a specific file.""" | ||
| return f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/{filename}" | ||
|
|
||
|
|
||
| def _fetch_raw(url: str) -> str | None: | ||
| def fetch_raw(url: str) -> str | None: | ||
| """GET *url* and return the response body as a string, or ``None`` on failure.""" |
Contributor
There was a problem hiding this comment.
Add Google-style docstrings for the newly public helpers.
raw_url and fetch_raw are now public APIs but still have private-helper style one-line docstrings. Please add Args: and Returns: sections.
Suggested docstring update
def raw_url(owner: str, repo: str, branch: str, filename: str) -> str:
- """Build a raw.githubusercontent.com URL for a specific file."""
+ """Build a raw.githubusercontent.com URL for a specific file.
+
+ Args:
+ owner: Repository owner or organization.
+ repo: Repository name.
+ branch: Branch name to fetch from.
+ filename: Filename within the repository root.
+
+ Returns:
+ Raw GitHub content URL for the requested file.
+ """
return f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/{filename}"
def fetch_raw(url: str) -> str | None:
- """GET *url* and return the response body as a string, or ``None`` on failure."""
+ """Fetch a URL and return the response body.
+
+ Args:
+ url: Raw content URL to fetch.
+
+ Returns:
+ Response body as text, or ``None`` on network/read failure.
+ """
try:As per coding guidelines: Every public module, class, and function must have a Google-style docstring with Args:, Returns:, and Raises: sections where relevant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch_hub/catalog/sources/__init__.py` around lines 59 - 65, Add
Google-style docstrings for the two public functions raw_url and fetch_raw:
replace the one-line docstrings with multi-line docstrings that include Args:
(document owner, repo, branch, filename for raw_url; url for fetch_raw),
Returns: (str for raw_url; str | None for fetch_raw) and Raises: where relevant
(e.g., document that fetch_raw may raise network/requests exceptions or return
None on failure). Ensure the docstrings follow Google style and are placed
immediately above the function definitions for raw_url and fetch_raw.
Comment on lines
+45
to
46
| _logger.exception("Config file '%s' not found", config_path) | ||
| sys.exit(1) |
Contributor
There was a problem hiding this comment.
Use warning/debug logging for the missing-config path.
This branch handles an expected CLI/config condition; logging a traceback via .exception(...) is overly noisy for this case.
Suggested change
try:
config = load_config(config_path)
except FileNotFoundError:
- _logger.exception("Config file '%s' not found", config_path)
+ _logger.warning("Config file '%s' not found", config_path)
sys.exit(1)As per coding guidelines: Log unexpected conditions using logger.warning(...) or logger.debug(...) from dfetch.log.get_logger(__name__).
📝 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
| _logger.exception("Config file '%s' not found", config_path) | |
| sys.exit(1) | |
| _logger.warning("Config file '%s' not found", config_path) | |
| sys.exit(1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch_hub/commands/__init__.py` around lines 45 - 46, Replace the noisy
traceback log call _logger.exception("Config file '%s' not found", config_path)
with a non-exception-level log (e.g., _logger.warning("Config file '%s' not
found", config_path) or _logger.debug(...)) in the same branch that exits via
sys.exit(1); this keeps the exit behavior (sys.exit(1)) but uses
_logger.warning/_logger.debug instead of _logger.exception to avoid emitting an
unnecessary traceback for an expected CLI/config condition.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 140-143: The exclude glob for doc8 is incorrect: replace the
current "*.dfetch_hub.egg-info/*" entry in the exclude list with a pattern that
matches the actual egg-info directory (e.g., "dfetch_hub.egg-info/*" or
"dfetch_hub.egg-info/**" to also exclude nested files). Update the exclude array
entry so the 'exclude' setting contains the corrected glob string to prevent
egg-info metadata from being linted.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyproject.toml (1)
123-128: Identify which file categories actually need exceptions before moving to per-file ignores.Per-file-ignores is indeed the recommended pattern in Ruff (v0.15.x) for rules that legitimately conflict with specific files or file categories (e.g., tests, migrations,
__init__.py). However, the suggestion to moveTRY003,TRY300, andRET505is incomplete without identifying which files or categories actually violate these rules.Before refactoring, determine:
- Do tests legitimately need
TRY003/TRY300/RET505exceptions?- Are there legacy files or specific modules where these patterns are unavoidable?
- Or do these rules conflict with your team's conventions throughout the codebase?
If specific file categories are identified, use explicit glob patterns:
Example per-file-ignores patterns
[tool.ruff.lint.per-file-ignores] "tests/**/*.py" = ["TRY003", "TRY300", "RET505"] "**/migrations/**/*.py" = ["TRY003", "TRY300"]If no specific categories are the culprit, these rules may legitimately remain global ignores (indicating they're not a fit for your project's style).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 123 - 128, Audit the codebase for occurrences of the Ruff rules TRY003, TRY300, and RET505 (e.g., by running ruff selects or a repo-wide search for patterns that trigger those codes) to determine which file categories legitimately need exceptions; if they are concentrated in tests, migrations, generated/legacy modules, or __init__.py files, add per-file ignores under [tool.ruff.lint.per-file-ignores] using explicit globs (for example mapping "tests/**/*.py" or "**/migrations/**/*.py" to the offending codes) instead of keeping those rules globally ignored, otherwise leave the codes in the global ignore list; ensure you reference the exact rule codes (TRY003, TRY300, RET505) and the config section name [tool.ruff.lint.per-file-ignores] when making changes.
🤖 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`:
- Around line 152-160: The pyright config currently sets typeCheckingMode =
"strict" but disables key diagnostics by setting reportUnknownVariableType,
reportUnknownArgumentType, and reportUnknownMemberType to "none"; change those
three keys to "warning" so strict mode still raises alerts for
unknown/partially-unknown types while preserving strict enforcement (edit the
entries for reportUnknownVariableType, reportUnknownArgumentType, and
reportUnknownMemberType in the pyproject.toml where typeCheckingMode is
defined).
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 123-128: Audit the codebase for occurrences of the Ruff rules
TRY003, TRY300, and RET505 (e.g., by running ruff selects or a repo-wide search
for patterns that trigger those codes) to determine which file categories
legitimately need exceptions; if they are concentrated in tests, migrations,
generated/legacy modules, or __init__.py files, add per-file ignores under
[tool.ruff.lint.per-file-ignores] using explicit globs (for example mapping
"tests/**/*.py" or "**/migrations/**/*.py" to the offending codes) instead of
keeping those rules globally ignored, otherwise leave the codes in the global
ignore list; ensure you reference the exact rule codes (TRY003, TRY300, RET505)
and the config section name [tool.ruff.lint.per-file-ignores] when making
changes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
spoorcc
force-pushed
the
tighten-screws
branch
2 times, most recently
from
96d5e4d to
1fed494
Compare
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
2 participants
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
Improvements
Documentation
Tests