Repository navigation
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:
📝 WalkthroughWalkthroughThis change adds changelog-based release automation and updates contributor and CI setup. It also adds download manifests, duration tracking, progress reporting, cache isolation, pagination state, task-context feature selection, export-path recording, tests, and documentation. ChangesDownload runtime and client APIs
Release automation and project setup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
.github/requirements-dev.txt (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the dev requirements or add a reproducible constraints file.
Unpinned dependencies can resolve to different releases on different Python matrix jobs and across future runs.
curl_cffikeeps Python 3.9-compatible releases below v0.14, while v0.14+ requires Python 3.10+, so thetest_api3.9 job can install a different release than the 3.10/3.13/3.14 jobs. Pin tested versions or enforce a shared constraints file before adding new matrix entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/requirements-dev.txt around lines 1 - 10, Make the dependencies in requirements-dev.txt reproducible by pinning each tested package to explicit compatible versions, including a curl_cffi version below 0.14 for Python 3.9, or add and apply one shared constraints file across all test_api Python matrix jobs. Ensure the selected versions install consistently on Python 3.9, 3.10, 3.13, and 3.14.src/jmcomic/api.py (1)
98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe four entry points repeat the same lifecycle block.
download_album,download_photo,download_album_async, anddownload_photo_asynceach repeat the identical sequence: readperf_counter, openjm_task_context, open the downloader, calladd_features, download, check exceptions, then call_finish_download_result. Only the download type, the entity kind, andawaitdiffer. Consider one sync helper and one async helper that take the download type and a callable. This reduces the chance that a future change updates three of the four sites.The current logic is correct, so treat this as optional cleanup.
Also applies to: 127-137, 207-216, 240-249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/api.py` around lines 98 - 108, Optionally consolidate the duplicated lifecycle logic from download_album, download_photo, download_album_async, and download_photo_async into one synchronous and one asynchronous helper. Have each helper accept the download type and a callable for the entity-specific download operation, while preserving feature registration, conditional exception checks, timing, task context, and _finish_download_result behavior.src/jmcomic/jm_downloader.py (1)
11-49: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
record_download_durationrequires the entity as a positional argument.The wrapper reads
entity = args[1]. If a caller passes the entity by keyword, for exampledownloader.download_by_image_detail(image=img)ordownloader.download_album(album_id='123'),args[1]raisesIndexError. All current callers pass positionally, so this is a robustness gap rather than a live defect.♻️ Proposed fix to tolerate keyword calls
def record_download_duration(context_key: str, clock=None): def decorator(func): def get_time(): return perf_counter() if clock is None else clock() + def read_entity(args, kwargs): + if len(args) > 1: + return args[1] + for value in kwargs.values(): + if isinstance(value, Downloadable): + return value + return None + if inspect.iscoroutinefunction(func): `@wraps`(func) async def async_wrapper(*args, **kwargs): - entity = args[1] + entity = read_entity(args, kwargs)Apply the same change to the synchronous wrapper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_downloader.py` around lines 11 - 49, Update record_download_duration’s async_wrapper and wrapper to resolve the entity from either the expected positional argument or its corresponding keyword argument, including calls such as image= or album_id=. Preserve the existing detail-call detection, context reuse, timing, and duration assignment behavior after resolving the entity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 28-42: Reorder the release workflow so the Build step runs before
Create Release, ensuring publication only occurs after a successful package
build. Apply the same ordering change to the corresponding steps in the release
workflow configuration, preserving the existing build and release settings.
- Around line 29-42: Update the release workflow action references for
softprops/action-gh-release and pypa/gh-action-pypi-publish in both release
workflows to reviewed, full immutable commit SHAs, replacing the mutable v2 and
release/v1 tags while preserving their existing configuration.
In @.github/workflows/test_api.yml:
- Line 39: Add .github/requirements-dev.txt to the on.push.paths filters in both
.github/workflows/test_api.yml (39-39) and .github/workflows/test_html.yml
(39-39), so changes to the shared development requirements trigger both test
matrices.
In `@assets/docs/sources/tutorial/14_async_usage.md`:
- Around line 259-260: Correct the Markdown table separator row in the section
containing the header “字段 | 它告诉你什么” so it defines exactly two columns, matching
the header row. Keep the table content and alignment unchanged.
In `@src/jmcomic/api.py`:
- Around line 19-21: Update _finish_download_result to avoid requiring every
downloader to populate manifest_dict[detail], and ensure DownloadResult.manifest
also handles missing manifests safely rather than raising KeyError. Preserve
duration updates when a manifest exists and keep returned download results valid
for custom download_album or download_photo implementations without manifest
lifecycle calls.
In `@src/jmcomic/jm_feature.py`:
- Around line 42-50: Update the tutorial examples for the current feature API:
change should_invoke calls to pass only when, and update _adapt_plugin_kwargs
examples to use the when parameter instead of the obsolete from argument. Remove
any feature_from usage while preserving the documented hook behavior.
In `@src/jmcomic/jm_plugin.py`:
- Line 804: Update both Img2pdfPlugin.invoke at src/jmcomic/jm_plugin.py:804-804
and LongImgPlugin.invoke at src/jmcomic/jm_plugin.py:887-887 consistently:
either guard downloader.record_export_filepath(...) when downloader is None, or
remove the None default and require downloader in both invoke signatures.
Preserve successful export behavior when a downloader is provided.
---
Nitpick comments:
In @.github/requirements-dev.txt:
- Around line 1-10: Make the dependencies in requirements-dev.txt reproducible
by pinning each tested package to explicit compatible versions, including a
curl_cffi version below 0.14 for Python 3.9, or add and apply one shared
constraints file across all test_api Python matrix jobs. Ensure the selected
versions install consistently on Python 3.9, 3.10, 3.13, and 3.14.
In `@src/jmcomic/api.py`:
- Around line 98-108: Optionally consolidate the duplicated lifecycle logic from
download_album, download_photo, download_album_async, and download_photo_async
into one synchronous and one asynchronous helper. Have each helper accept the
download type and a callable for the entity-specific download operation, while
preserving feature registration, conditional exception checks, timing, task
context, and _finish_download_result behavior.
In `@src/jmcomic/jm_downloader.py`:
- Around line 11-49: Update record_download_duration’s async_wrapper and wrapper
to resolve the entity from either the expected positional argument or its
corresponding keyword argument, including calls such as image= or album_id=.
Preserve the existing detail-call detection, context reuse, timing, and duration
assignment behavior after resolving the entity.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b94a071e-5b44-4981-8aa4-bb9282205773
📒 Files selected for processing (39)
.github/CONTRIBUTING.md.github/release.py.github/requirements-dev.txt.github/workflows/benchmark.yml.github/workflows/download.yml.github/workflows/download_dispatch.yml.github/workflows/export_favorites.yml.github/workflows/release.yml.github/workflows/release_auto.yml.github/workflows/test_api.yml.github/workflows/test_html.yml.gitignoreCHANGELOG.mdassets/docs/mkdocs.ymlassets/docs/sources/index.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/11_log_custom.mdassets/docs/sources/tutorial/14_async_usage.mdassets/docs/sources/tutorial/4_module_custom.mdassets/docs/sources/tutorial/6_plugin.mdassets/docs/sources/tutorial/7_advance.mdassets/docs/sources/tutorial/9_custom_download_dir_name.mdsrc/jmcomic/__init__.pysrc/jmcomic/api.pysrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_async_feature.pytests/test_jmcomic/test_jm_client.pytests/test_jmcomic/test_jm_download_manifest.pytests/test_jmcomic/test_jm_feature.pytests/test_jmcomic/test_jm_release.pytests/test_jmcomic/test_jm_task_context.py
💤 Files with no reviewable changes (3)
- assets/docs/sources/index.md
- assets/docs/sources/tutorial/7_advance.md
- assets/docs/sources/tutorial/4_module_custom.md
| uses: softprops/action-gh-release@v2 | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| with: | ||
| tag_name: ${{ steps.tb.outputs.tag }} | ||
| body_path: release_body.txt | ||
|
|
||
| - name: Build | ||
| run: | | ||
| python -m pip install build | ||
| python -m build | ||
|
|
||
| - name: Publish PYPI | ||
| - name: Release PYPI | ||
| uses: pypa/gh-action-pypi-publish@release/v1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-remote https://github.com/softprops/action-gh-release refs/tags/v2
git ls-remote https://github.com/pypa/gh-action-pypi-publish refs/tags/release/v1Repository: hect0x7/JMComic-Crawler-Python
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Workflow file context:\n'
sed -n '1,90p' .github/workflows/release.yml 2>/dev/null || true
printf '\nAll action uses in workflows:\n'
rg -n 'uses:\s*[^#]+' .github/workflows || trueRepository: hect0x7/JMComic-Crawler-Python
Length of output: 3084
🌐 Web query:
softprops action-gh-release v2 documentation permissions releases token write
💡 Result:
To use softprops/action-gh-release, you must configure your GitHub Actions workflow with the necessary permissions to allow the action to create releases [1][2]. Required Permissions The action requires the contents: write permission [1][3]. If you also use the discussion_category_name input, you must additionally provide the discussions: write permission [1][3]. You can set these permissions at the workflow or job level in your YAML configuration: permissions: contents: write If enabling discussions: permissions: contents: write discussions: write Important Token Considerations 1. Default Token: By default, the action uses the standard GITHUB_TOKEN [2][4]. Ensure that your repository settings (under Actions > General > Workflow Permissions) allow for Read and Write access [5]. 2. Personal Access Token (PAT): If you are running the workflow on the release event (e.g., on: { release: { types: [published] } }), you must provide a personal access token instead of the default GITHUB_TOKEN, because the default token will not trigger subsequent workflows [1][2]. 3. Overriding: You can explicitly pass a custom token using the token input if needed [2][6]. A non-empty explicit token will override the default GITHUB_TOKEN [4][6]. Note that v2 of this action is no longer maintained [2]. While the permission requirements remain the same, it is recommended to review the project's current status for migration paths [2].
Citations:
- 1: https://github.com/softprops/action-gh-release/blob/v2/README.md
- 2: https://github.com/softprops/action-gh-release
- 3: https://github.com/softprops/action-gh-release/blob/master/README.md
- 4: https://github.com/softprops/action-gh-release?tab=readme-ov-file
- 5: Getting 403 on this workflow softprops/action-gh-release#366
- 6: https://github.com/softprops/action-gh-release/blob/master/action.yml
🌐 Web query:
pypa gh-action-pypi-publish release/v1 OIDC token packages publish README
💡 Result:
The pypa/gh-action-pypi-publish action (specifically the release/v1 branch) supports Trusted Publishing, which uses OpenID Connect (OIDC) to authenticate with PyPI without needing manual API tokens or passwords [1][2][3]. To use OIDC/Trusted Publishing with this action, you must follow these requirements: 1. Configure Trusted Publishing on PyPI: Your project on PyPI must already be configured to trust your GitHub repository as a publisher [1][3]. 2. Set Permissions: The GitHub Actions job running the publish step must have the id-token: write permission enabled [1][4]. 3. Omit Credentials: Do not provide an explicit username or password (or API token) to the action [1][2]. Example configuration for your GitHub Actions workflow: jobs: pypi-publish: name: Upload release to PyPI runs-on: ubuntu-latest environment: name: pypi permissions: id-token: write # Mandatory for Trusted Publishing steps: #... retrieve your distributions (e.g., download-artifact)... - name: Publish package distributions to PyPI uses: pypa/gh-action-pypi-publish@release/v1 Additionally, this action automatically generates and uploads PEP 740-compatible attestations using Sigstore, which uses the same OIDC identity [1][5]. If you need to use a self-hosted runner, Trusted Publishing is generally expected to work, provided it is configured correctly with GitHub [1][3].
Citations:
- 1: https://github.com/pypa/gh-action-pypi-publish?tab=readme-ov-file
- 2: https://github.com/pypa/gh-action-pypi-publish
- 3: https://github.com/marketplace/actions/pypi-publish
- 4: https://docs.pypi.org/trusted-publishers/using-a-publisher/
- 5: https://packaging.python.org/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Inspect Git references:\n'
git ls-remote https://github.com/softprops/action-gh-release refs/tags/v2 || true
git ls-remote https://github.com/softprops/action-gh-release refs/heads/v2 || true
git ls-remote https://github.com/softprops/action-gh-release refs/tags/v1 || true
git ls-remote https://github.com/pypa/gh-action-pypi-publish refs/tags/release/v1 || true
git ls-remote https://github.com/pypa/gh-action-pypi-publish refs/heads/release/v1 || true
printf '\nWorkflow permission blocks:\n'
rg -n -C 3 'permissions:|id-token|contents|Create Release|Release PYPI|GitHub Release|pypi-publish|action-gh-release' .github/workflows || trueRepository: hect0x7/JMComic-Crawler-Python
Length of output: 3297
Pin the release actions to immutable commit SHAs.
softprops/action-gh-release@v2 runs with contents: write, and pypa/gh-action-pypi-publish@release/v1 runs with id-token: write for PyPI Trusted Publishing. Update both uses in .github/workflows/release.yml and .github/workflows/release_auto.yml to reviewed full commit SHAs instead of mutable tags.
🧰 Tools
🪛 zizmor (1.29.0)
[info] 29-29: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 29 - 42, Update the release
workflow action references for softprops/action-gh-release and
pypa/gh-action-pypi-publish in both release workflows to reviewed, full
immutable commit SHAs, replacing the mutable v2 and release/v1 tags while
preserving their existing configuration.
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)
tests/test_jmcomic/test_jm_release.py (1)
117-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove
workflow_dispatchfromrelease_auto.yml.
release_auto.ymldeclares a manual dispatch, but its job condition only matchesgithub.event.head_commit.message, which aworkflow_dispatchevent does not provide. This makes the manual execution path inrelease_auto.ymlskipped unlessrelease.ymlis used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_release.py` around lines 117 - 124, Remove the workflow_dispatch trigger declaration from release_auto.yml, leaving only the push trigger used by the existing release job condition. Update the release workflow test around test_workflow_keeps_master_v_prefix_trigger_without_generated_notes to assert that workflow_dispatch is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/docs/sources/tutorial/13_export_and_feature.md`:
- Line 185: Update the diagram step around option.invoke to match the
implemented invocation in invoke_plugin: show the registered plugin class,
adapted arguments, event extras, and plugin metadata, or clearly label the
simplified call as pseudocode.
In `@tests/test_jmcomic/test_jm_download_manifest.py`:
- Around line 240-246: Update test_export_plugins_allow_omitting_downloader to
create an isolated TemporaryDirectory and build all temporary file paths with
os.path.join instead of fixed /tmp literals. Replace the hard-coded paths in
ContractOption and the cases, preserving each plugin’s expected relative
filenames and assertions within the temporary directory.
---
Outside diff comments:
In `@tests/test_jmcomic/test_jm_release.py`:
- Around line 117-124: Remove the workflow_dispatch trigger declaration from
release_auto.yml, leaving only the push trigger used by the existing release job
condition. Update the release workflow test around
test_workflow_keeps_master_v_prefix_trigger_without_generated_notes to assert
that workflow_dispatch is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9828a414-921f-4e41-92a4-3ea71b39d757
📒 Files selected for processing (12)
.github/workflows/release.yml.github/workflows/release_auto.yml.github/workflows/test_api.yml.github/workflows/test_html.ymlassets/docs/sources/tutorial/13_export_and_feature.mdassets/docs/sources/tutorial/14_async_usage.mdsrc/jmcomic/api.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_plugin.pytests/test_jmcomic/test_jm_download_manifest.pytests/test_jmcomic/test_jm_release.pytests/test_jmcomic/test_jm_task_context.py
💤 Files with no reviewable changes (1)
- tests/test_jmcomic/test_jm_task_context.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/test_html.yml
- .github/workflows/test_api.yml
- .github/workflows/release_auto.yml
- src/jmcomic/jm_plugin.py
- .github/workflows/release.yml
- assets/docs/sources/tutorial/14_async_usage.md
- src/jmcomic/api.py
- src/jmcomic/jm_downloader.py
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)
src/jmcomic/jm_downloader.py (1)
273-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake manifest absence safe at the public boundary.
record_export_filepathraises when no active manifest exists. Export plugin paths call it without guaranteeing the caller set up a manifest. Make it non-throwing when no manifest is active.
DownloadResult.manifest,DownloadResult.duration, and_finish_download_resultassumeself.downloader.manifest_dict[self.detail]exists. Initialize a manifest before creating the result, or return safe missing-state values for these result accesses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_downloader.py` around lines 273 - 277, Update record_export_filepath so a missing resolve_manifest_detail result returns safely without raising. Also adjust _finish_download_result and DownloadResult.manifest/ duration accesses to handle absent self.downloader.manifest_dict[self.detail], preferably by initializing the manifest before result creation or returning safe missing-state values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jmcomic/jm_downloader.py`:
- Line 13: Resolve the Ruff RUF003 warnings in the comments near the
parameter-parsing logic by replacing the ambiguous full-width comma and
semicolon punctuation with ASCII equivalents. Update the affected comments at
the referenced locations and do not alter the surrounding implementation.
---
Outside diff comments:
In `@src/jmcomic/jm_downloader.py`:
- Around line 273-277: Update record_export_filepath so a missing
resolve_manifest_detail result returns safely without raising. Also adjust
_finish_download_result and DownloadResult.manifest/ duration accesses to handle
absent self.downloader.manifest_dict[self.detail], preferably by initializing
the manifest before result creation or returning safe missing-state values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d0c0c62-fd20-400e-9091-436c7cdd0757
📒 Files selected for processing (2)
src/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_downloader.py
|
|
||
| def record_download_duration(context_key: str, clock=None): | ||
| def decorator(func): | ||
| # 装饰时只解析一次参数名，关键字调用无需在每次执行时重复 inspect。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Ruff RUF003 warnings.
Ruff flags ambiguous full-width punctuation in the comments at Lines [13], [20], [30], [37], [48], and [55]. Replace ， and ； with ASCII punctuation, or configure an explicit exception.
Also applies to: 20-20, 30-30, 37-37, 48-48, 55-55
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 13-13: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_downloader.py` at line 13, Resolve the Ruff RUF003 warnings in
the comments near the parameter-parsing logic by replacing the ambiguous
full-width comma and semicolon punctuation with ASCII equivalents. Update the
affected comments at the referenced locations and do not alter the surrounding
implementation.
Source: Linters/SAST tools
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
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)
src/jmcomic/jm_async_downloader.py (1)
183-183: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the new Ruff
RUF003warnings.Replace ambiguous full-width punctuation in these comments with ASCII punctuation, or configure an explicit exception.
Also applies to: 199-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_async_downloader.py` at line 183, Update the comments around the asynchronous image download flow and lines 199-201 to replace ambiguous full-width punctuation with equivalent ASCII punctuation, resolving Ruff RUF003 without changing runtime behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jmcomic/jm_downloader.py`:
- Around line 30-35: Update the timing wrapper around the detail download flow
in jm_downloader.py so detail.duration is assigned in a finally path, including
when the wrapped photo or image download raises. Preserve the shared
timing-context behavior for nested detail calls, and add tests covering failed
detail attempts with a recorded duration.
---
Outside diff comments:
In `@src/jmcomic/jm_async_downloader.py`:
- Line 183: Update the comments around the asynchronous image download flow and
lines 199-201 to replace ambiguous full-width punctuation with equivalent ASCII
punctuation, resolving Ruff RUF003 without changing runtime behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea549967-7496-4c17-a60e-5f874ba83570
📒 Files selected for processing (2)
src/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_downloader.py
| # 顶层 ID 下载负责完整耗时，内部 detail 调用复用同一个计时上下文。 | ||
| if detail_call and get_jm_task_context().get(context_key) is not None: | ||
| return await func(*args, **kwargs) | ||
|
|
||
| started_at = get_time() | ||
| with jm_task_context(**{context_key: started_at}): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record durations for failed detail attempts.
Both wrapper branches assign detail.duration only after the wrapped call returns. If a photo or image download raises, the assignment is skipped. The failure handlers retain the entity with duration=None. Record detail duration in a finally path and add failure-case tests. (github.com)
Also applies to: 47-51
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 30-30: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_downloader.py` around lines 30 - 35, Update the timing wrapper
around the detail download flow in jm_downloader.py so detail.duration is
assigned in a finally path, including when the wrapped photo or image download
raises. Preserve the shared timing-context behavior for nested detail calls, and
add tests covering failed detail attempts with a recorded duration.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/docs/sources/tutorial/13_export_and_feature.md`:
- Line 115: Update the internal link in the note near the “download_album” and
“download_photo” references to target the generated anchor for the “2.5 智能适配规则”
heading, replacing the incorrect `#25` fragment with the heading’s actual anchor.
In `@src/jmcomic/jm_plugin.py`:
- Around line 756-767: Make lazy progress initialization atomic in
ProgressDownloader.__init__/before_photo and
AsyncProgressDownloader.before_photo. At src/jmcomic/jm_plugin.py lines 756-767,
add and use a dedicated instance lock around the progress None check and
start_progress call; apply the same guard at lines 852-863, preserving the async
superclass flow so concurrent chapters retain all task entries.
- Around line 954-956: Update redirect_log_to_file to detach existing jm_logger
handlers without calling close(), since they are not plugin-owned. Preserve the
detached handlers in a local or instance-level reference, and restore them when
the redirected logging configuration is no longer active, maintaining the
logger’s prior state.
- Around line 810-817: Update stop_progress so progress.stop() is invoked for
both interactive and disabled progress before clearing self.progress; remove the
progress.console.is_interactive guard while preserving the existing
unregister_progress ordering and None handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97c5293c-35a0-4869-a3f2-2eb76b508467
⛔ Files ignored due to path filters (1)
assets/docs/sources/images/download_progress_terminal.pngis excluded by!**/*.png
📒 Files selected for processing (22)
CHANGELOG.mdassets/docs/sources/index.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/12_domain_strategy.mdassets/docs/sources/tutorial/13_export_and_feature.mdassets/docs/sources/tutorial/15_download_progress.mdassets/docs/sources/tutorial/6_plugin.mdsrc/jmcomic/api.pysrc/jmcomic/cl.pysrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_client_interface.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_cli.pytests/test_jmcomic/test_jm_client.pytests/test_jmcomic/test_jm_download_progress.py
💤 Files with no reviewable changes (2)
- src/jmcomic/cl.py
- tests/test_jmcomic/test_jm_cli.py
🚧 Files skipped from review as they are similar to previous changes (4)
- assets/docs/sources/tutorial/6_plugin.md
- src/jmcomic/api.py
- src/jmcomic/jm_async_downloader.py
- src/jmcomic/jm_downloader.py
| def before_photo(self, photo): | ||
| super().before_photo(photo) | ||
| if getattr(self, 'progress', None) is None: | ||
| self.start_progress() | ||
| with self.progress_lock: | ||
| self.chapter_done[photo.id] = 0 | ||
| self.chapter_total[photo.id] = len(photo) | ||
| self.chapter_tasks[photo.id] = self.progress.add_task( | ||
| f'[cyan]章节-{self.display_id(photo.id)}[/cyan]', | ||
| total=len(photo), | ||
| ) | ||
| self.refresh_progress() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded lazy start_progress in both progress downloaders. Both classes check self.progress for None and then call start_progress without an atomic guard. start_progress replaces progress_lock, album_total, album_done, chapter_done, chapter_total, and chapter_tasks. Concurrent chapters can therefore both initialize, and the losing chapter's task entry disappears, which makes after_image raise KeyError.
src/jmcomic/jm_plugin.py#L756-L767: wrap thegetattr(self, 'progress', None) is Nonecheck and thestart_progress()call inProgressDownloader.before_photowith a dedicated instance lock created in__init__.src/jmcomic/jm_plugin.py#L852-L863: apply the same guard inAsyncProgressDownloader.before_photo, whereawait super().before_photo(photo)creates the interleaving point between the check and the initialization.
📍 Affects 1 file
src/jmcomic/jm_plugin.py#L756-L767(this comment)src/jmcomic/jm_plugin.py#L852-L863
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_plugin.py` around lines 756 - 767, Make lazy progress
initialization atomic in ProgressDownloader.__init__/before_photo and
AsyncProgressDownloader.before_photo. At src/jmcomic/jm_plugin.py lines 756-767,
add and use a dedicated instance lock around the progress None check and
start_progress call; apply the same guard at lines 852-863, preserving the async
superclass flow so concurrent chapters retain all task entries.
| def stop_progress(self): | ||
| progress = getattr(self, 'progress', None) | ||
| if progress is None: | ||
| return | ||
| self.unregister_progress(progress) | ||
| if progress.console.is_interactive: | ||
| progress.stop() | ||
| self.progress = None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
rich Progress disable=True does start() begin the Live display and is stop() required
💡 Result:
When you initialize a Rich Progress object with disable=True, calling start does not begin the Live display [1][2]. The disable flag effectively prevents the underlying Live display from being started [1][2]. Regarding whether stop is required: while calling start on a disabled Progress object does not initiate a display, it is still standard best practice to use the Progress object as a context manager (using the with statement) [3][4]. This ensures that all lifecycle methods are handled correctly regardless of the disable state [3][4]. If you choose to manage the lifecycle manually (without a context manager), you should still call stop to ensure consistency and prevent potential resource leaks or unexpected behavior, even if the display is disabled [3][4]. In summary: 1. start does not start the Live display when disable=True [1][2]. 2. stop should still be called (or the context manager used) to maintain proper object lifecycle management [3][4].
Citations:
- 1: https://rich.readthedocs.io/en/stable/%5Fmodules/rich/progress.html
- 2: https://github.com/willmcgugan/rich/blob/master/rich/progress.py
- 3: https://rich.readthedocs.io/en/stable/progress.html
- 4: https://github.com/Textualize/rich/blob/fc41075a/docs/source/progress.rst
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'jm_plugin\.py|.*progress.*' . | sed 's#^\./##' | head -50
echo
echo "== jm_plugin.py around new_rich_progress and stop_progress =="
if [ -f src/jmcomic/jm_plugin.py ]; then
nl -ba src/jmcomic/jm_plugin.py | sed -n '760,840p'
fi
echo
echo "== rich dependency/version references =="
for f in pyproject.toml setup.py setup.cfg requirements*.txt Pipfile poetry.lock; do
[ -f "$f" ] && { echo "--- $f"; rg -n "rich|jmcomic" "$f" || true; }
done
echo
echo "== rich availability/version =="
python3 - <<'PY'
import importlib.util, sys
spec = importlib.util.find_spec("rich")
print("available:", bool(spec), "spec:", spec)
if spec:
import rich
print("version:", getattr(rich, "__version__", None))
try:
import inspect
print("progress start line:", inspect.getsourcefile(rich.progress.Progress.start))
except Exception as e:
print("progress start source exception:", type(e).__name__, e)
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== jm_plugin.py around progress == "
python3 - <<'PY'
from pathlib import Path
p=Path('src/jmcomic/jm_plugin.py')
lines=p.read_text(encoding='utf-8').splitlines()
for i in range(735, 835):
if i <= len(lines):
print(f"{i:5}: {lines[i-1]}")
PY
echo
echo "== rich dependency/version references =="
for f in pyproject.toml setup.py setup.cfg requirements*.txt requirements.txt Pipfile poetry.lock; do
[ -f "$f" ] && { echo "--- $f"; grep -n -i "rich\|jmcomic" "$f" || true; }
done
echo
echo "== rich availability/version/source behavior probe =="
python3 - <<'PY'
import importlib.util, inspect, sys
spec = importlib.util.find_spec("rich")
print("available:", bool(spec))
print("spec:", spec)
if spec:
import rich
print("version:", getattr(rich, "__version__", None))
from rich.console import Console
from rich.progress import Progress
console = Console(force_terminal=True, width=80)
prog = Progress(console=console, disable=True)
print("disabled progress disable:", prog.disable)
print("disabled progress console is_interactive:", console.is_interactive)
# Inspect the relevant source without executing repository code.
src = inspect.getsource(prog.start)
print("start source:")
for num, line in enumerate(src.splitlines(), 1):
if "if self.disable" in line or "super" in line or "live" in line.lower() or "__enter__" in line:
print(f"{num}: {line}")
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 5439
🌐 Web query:
Rich progress.py Progress start() disable self.live is None stop() source
💡 Result:
In the Rich library, the Progress class manages progress bar displays, often utilizing a Live object to handle rendering [1][2]. The start and stop methods control the lifecycle of this progress display, with internal checks to ensure they behave correctly based on the configuration of the Progress instance [1][3]. Specifically, the start and stop methods check the self.disable attribute [1][2]: start: def start(self) -> None: """Start the progress display.""" if not self.disable: self.live.start(refresh=True) stop: def stop(self) -> None: """Stop the progress display.""" if not self.disable: self.live.stop if not self.console.is_interactive and not self.console.is_jupyter: self.console.print When a Progress instance is initialized with disable=True, the disable attribute is set accordingly [1][4]. Because these methods verify if the instance is disabled before interacting with self.live, calling start or stop on a disabled Progress instance will not initiate or interact with the Live display, effectively preventing any output [1][5][2]. The self.live object is initialized during the instantiation of the Progress class and is managed by the Progress instance itself; it is not None by default, but its active use is gated by the disable flag [1][4][2]. If you are observing an issue where self.live appears to be None, it is not the standard behavior of the library, as the Progress constructor assigns a Live object to self.live [1][2].
Citations:
- 1: https://github.com/Textualize/rich/blob/main/rich/progress.py
- 2: https://github.com/Textualize/rich/blob/9d8f9a37/rich/progress.py
- 3: https://github.com/Textualize/rich/blob/master/rich/progress.py
- 4: https://rich.readthedocs.io/en/stable/reference/progress.html
- 5: Disabled Progress should not start attached Live Textualize/rich#1125
🌐 Web query:
"rich.progress" "def start" "if not self.disable" "console.print"
💡 Result:
In the rich library's Progress class, the start method checks the disable attribute before starting the live display [1][2]. The implementation is as follows: def start(self) -> None: """Start the progress display.""" if not self.disable: self.live.start(refresh=True) This check ensures that if the Progress instance is initialized with disable=True, the live display (which handles rendering the progress bar) is not started [1][3][4]. Regarding your mention of console.print, the Progress class manages an internal Console object [5][6]. While the start method does not directly call console.print, the stop method contains logic that conditionally calls console.print when the progress display is finished, depending on whether the console is interactive or running in a Jupyter environment [1][2][7]. Some users have noted that when a progress bar is disabled, certain behaviors related to these console operations can still occur, sometimes resulting in unexpected output or blank lines [8].
Citations:
- 1: https://github.com/willmcgugan/rich/blob/master/rich/progress.py
- 2: https://github.com/Textualize/rich/blob/9d8f9a37/rich/progress.py
- 3: https://rich.readthedocs.io/en/latest/reference/progress.html
- 4: https://rich.readthedocs.io/en/stable/_modules/rich/progress.html
- 5: https://rich.readthedocs.io/en/stable/progress.html
- 6: https://github.com/Textualize/rich/blob/fc41075a/docs/source/progress.rst
- 7: https://github.com/Textualize/rich/blob/4d6d631a/rich/progress.py
- 8: [BUG] disabled progress bar has unexpected behavior that breaks doctests Textualize/rich#3103
Call progress.stop() even for disabled progress.
start_progress() calls progress.start() regardless of disable, and Progress.start() only avoids starting Live when disable=True. Progress.stop() also checks disable before any lifecycle cleanup, so the non-interactive path must call stop() before clearing self.progress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_plugin.py` around lines 810 - 817, Update stop_progress so
progress.stop() is invoked for both interactive and disabled progress before
clearing self.progress; remove the progress.console.is_interactive guard while
preserving the existing unregister_progress ordering and None handling.
| for old_handler in jm_logger.handlers[:]: | ||
| jm_logger.removeHandler(old_handler) | ||
| old_handler.close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not close logging handlers that the plugin does not own.
redirect_log_to_file removes every handler from the shared jmcomic logger and calls close() on each one. The plugin does not own those handlers. A host application that attached its own handler loses it permanently, and any later write to that handler fails because the stream is closed. The plugin also never restores the previous state.
Detach the handlers without closing them, and keep a reference so the previous configuration can be restored.
🛡️ Proposed fix for handler ownership
- for old_handler in jm_logger.handlers[:]:
- jm_logger.removeHandler(old_handler)
- old_handler.close()
+ self.replaced_handlers = jm_logger.handlers[:]
+ for old_handler in self.replaced_handlers:
+ jm_logger.removeHandler(old_handler)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jmcomic/jm_plugin.py` around lines 954 - 956, Update redirect_log_to_file
to detach existing jm_logger handlers without calling close(), since they are
not plugin-owned. Preserve the detached handlers in a local or instance-level
reference, and restore them when the redirected logging configuration is no
longer active, maintaining the logger’s prior state.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_jmcomic/test_jm_client.py (3)
402-429: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the parsed comment IDs.
The fixture sets
data-cid="100"anddata-cid="101", but the test only checks user and album IDs. A regression incomment_idparsing would still pass.Proposed assertions
self.assertEqual(page.page_number, 1) + self.assertEqual(str(page[0].comment_id), '100') + self.assertEqual(str(page[1].comment_id), '101') self.assertEqual(page[0].user_id, '200')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_client.py` around lines 402 - 429, Update test_html_forum_comment_id_parsing to assert each parsed comment’s comment_id matches the fixture’s data-cid values: 100 for page[0] and 101 for page[1], while preserving the existing user_id and album_id assertions.
453-458: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest nested cache isolation, not only outer-object identity.
assertIsNot(ans, photo)does not detect a shallow copy. Comparingtagswithout mutating it also allows both results to share the same mutable list. Mutate a nested field and verify that the other cached result remains unchanged.Proposed test extension
self.assertEqual(ans.tags, photo.tags) + original_tags = list(photo.tags) + try: + photo.tags.append('__cache_isolation__') + self.assertEqual(ans.tags, original_tags) + finally: + photo.tags[:] = original_tags🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_client.py` around lines 453 - 458, Extend the cache comparison in the test branch around ans and photo to verify nested-object isolation: after confirming the existing fields, mutate the mutable tags collection on one result and assert the other result’s tags remain unchanged. Preserve the current identity and value assertions while ensuring the test would fail for a shallow copy.
554-562: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the synchronous generator explicitly.
The test leaves
JmcomicClient.do_page_itersuspended aftersend. The asynchronous test callsaclose(), but the synchronous test does not callclose(). Close the generator in afinallyblock so generator cleanup and task-context cleanup do not depend on garbage collection.Proposed cleanup
generator = JmcomicClient.do_page_iter(None, {}, 1, get_page) - - self.assertEqual(next(generator).page_number, 1) - self.assertEqual(generator.send({'page': 3}).page_number, 3) + try: + self.assertEqual(next(generator).page_number, 1) + self.assertEqual(generator.send({'page': 3}).page_number, 3) + finally: + generator.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_client.py` around lines 554 - 562, Update test_page_number_in_sync_generator to close the generator explicitly in a finally block surrounding the next/send assertions, ensuring JmcomicClient.do_page_iter is closed even if an assertion fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_jmcomic/test_jm_client.py`:
- Around line 402-429: Update test_html_forum_comment_id_parsing to assert each
parsed comment’s comment_id matches the fixture’s data-cid values: 100 for
page[0] and 101 for page[1], while preserving the existing user_id and album_id
assertions.
- Around line 453-458: Extend the cache comparison in the test branch around ans
and photo to verify nested-object isolation: after confirming the existing
fields, mutate the mutable tags collection on one result and assert the other
result’s tags remain unchanged. Preserve the current identity and value
assertions while ensuring the test would fail for a shallow copy.
- Around line 554-562: Update test_page_number_in_sync_generator to close the
generator explicitly in a finally block surrounding the next/send assertions,
ensuring JmcomicClient.do_page_iter is closed even if an assertion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e84efac-cdf9-4464-b1e6-43dbfad078b6
📒 Files selected for processing (3)
src/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_client.pytests/test_jmcomic/test_jm_download_progress.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/jmcomic/jm_toolkit.py
|
@coderabbitai review |
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/jmcomic/jm_entity.py (1)
720-725: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep
page_numberbehindraw_htmlandraw_datafor positional compatibility.
JmAlbumCommentPage.__init__still accepts positional arguments, but the new signature makes existing positional constructions, such asJmAlbumCommentPage([comment]), pack positional values into the wrong fields. Movepage_numberafter the existing optional parameters, ideally as a keyword-only argument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_entity.py` around lines 720 - 725, Update JmAlbumCommentPage.__init__ so page_number follows the existing raw_html and raw_data parameters, preferably making it keyword-only, while preserving current positional argument mappings and page_number behavior.src/jmcomic/jm_downloader.py (1)
309-344: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
_feature_listis not reset per top-level download.
add_featuresappends toself._feature_list, and nothing clears it. If one downloader instance runs several top-level downloads, features from the earlier download run again. Consider clearing the list inbegin_manifest, or documenting that one downloader serves one top-level download.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_downloader.py` around lines 309 - 344, Reset _feature_list at the start of each top-level download by clearing it in begin_manifest before features are registered or executed. Preserve add_features behavior and ensure subsequent downloads on the same downloader instance do not reuse features from earlier runs.tests/test_jmcomic/test_jm_download_manifest.py (1)
774-774: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefix the unused unpacked variables with an underscore.
Ruff
RUF059flagsphotoat Line 774,albumat Line 989, andoptionat Line 1003. None of them are used in their test bodies.Also applies to: 989-989, 1003-1003
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_download_manifest.py` at line 774, In the test cases using self.new_downloader, prefix the unused unpacked variables photo, album, and option with underscores at the identified assignments, while preserving the variables that are used by each test.Source: Linters/SAST tools
tests/test_jmcomic/test_jm_task_context.py (1)
299-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
create=Truein these patches.
create=Truemakespatchadd the attribute whenjmcomic.apidoes not importperf_counter. If the import is renamed or removed, the patch stops affecting the production code and the test fails with a confusing duration mismatch instead of anAttributeError. Removecreate=Trueso the patch target is validated.♻️ Proposed change
- with patch('jmcomic.api.perf_counter', side_effect=lambda: clock['now'], create=True): + with patch('jmcomic.api.perf_counter', side_effect=lambda: clock['now']):Also applies to: 336-336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_task_context.py` at line 299, Remove create=True from the patches targeting jmcomic.api.perf_counter in the affected test cases, so unittest.mock validates that the production module exposes perf_counter and raises an AttributeError if the import changes.tests/test_jmcomic/test_jm_client.py (1)
554-575: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese tests call
do_page_iterunbound withNoneasself.The calls work only while the implementations ignore
self. Ifdo_page_iterstarts using instance state, the tests fail with anAttributeErroronNone. Consider a minimal stub client instance instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_client.py` around lines 554 - 575, Update test_page_number_in_sync_generator and test_page_number_in_async_generator to invoke do_page_iter through minimal JmcomicClient and AsyncJmcomicClient instances instead of passing None as self. Preserve the existing pagination inputs and page-number assertions while ensuring both tests use valid client objects.src/jmcomic/jm_feature.py (1)
80-93: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
should_invokereturnsFalsewhen the task context has nodownload_type.
when == f'after_{download_type}'builds'after_None'when the key is missing. The downloader path already enforces the context, so this is not reachable there. For direct calls the feature silently does nothing. Consider an explicit guard that states the missing context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_feature.py` around lines 80 - 93, Update should_invoke to explicitly validate that jm_task_context contains a download_type before constructing the expected after_<type> hook name; when it is missing, raise a clear error identifying the absent context instead of returning False, while preserving the existing matching behavior for valid download types.tests/test_jmcomic/test_jm_release.py (1)
117-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThese assertions depend on exact whitespace and prose.
test_workflow_keeps_master_v_prefix_trigger_without_generated_notesmatches"branches:\n - master", andtest_test_workflows_watch_development_requirementsmatches a six-space indented line. A reformat of the YAML breaks the tests without any behavior change. TheCONTRIBUTING.mdassertions break on any wording edit.Parse the workflow files with a YAML loader and assert on the parsed structure. Keep the documentation assertions loose, or drop them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_release.py` around lines 117 - 155, Replace brittle raw-text assertions in test_workflow_keeps_master_v_prefix_trigger_without_generated_notes, test_manual_workflow_reads_source_version_from_master, test_release_workflows_build_before_creating_release, and test_test_workflows_watch_development_requirements with assertions against parsed YAML structures and relevant semantic values. In test_contributing_allows_only_formal_release_prs_to_master, avoid exact prose matching by asserting stable semantic content or remove the wording-dependent checks.src/jmcomic/jm_plugin.py (1)
824-917: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared progress state into a mixin.
AsyncProgressDownloader.start_progress,before_photo,after_image,after_photo,after_albumandstop_progressduplicate theProgressDownloaderbodies. The only difference isself.versusProgressDownloader.qualification and theawait super()calls. The code already calls unbound methods such asProgressDownloader.refresh_progress(self)andProgressDownloader.print_non_interactive_summary(self), which shows the state handling is class independent. Move the state fields and the pure state helpers into aProgressStateMixinand let both downloaders inherit it. This removes the risk that a future fix lands in only one of the two classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jmcomic/jm_plugin.py` around lines 824 - 917, The progress state logic is duplicated between AsyncProgressDownloader and ProgressDownloader. Extract shared fields and state-only methods (start_progress, before_photo, after_image, after_photo, after_album, and stop_progress) into a ProgressStateMixin, then have both downloader classes inherit it; keep async lifecycle methods’ await super() behavior and use the mixin’s instance helpers without changing progress behavior.tests/test_jmcomic/test_jm_download_progress.py (1)
235-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting Rich panel line counts.
richis unlocked in.github/requirements-dev.txt, and Rich output dimensions are not guaranteed stable across releases. Line 237 can fail on a Rich update even when plugin behavior is unchanged. The surrounding content assertions already cover the rendered notice; remove or replace the exact count check with a behavior-based criterion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_download_progress.py` around lines 235 - 249, Remove the exact rendered line-count assertion in the test around the UI output checks. Keep the existing content-based assertions for the JMComic progress panel, or replace the count check with a behavior-based assertion that remains stable across Rich versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 33-42: Reorder the release jobs so the existing “Release PYPI”
step runs after “Build” and before “Create Release” in both workflow files.
Preserve the current publishing and GitHub release configuration while ensuring
the public GitHub release is created only after PyPI publishing succeeds.
---
Nitpick comments:
In `@src/jmcomic/jm_downloader.py`:
- Around line 309-344: Reset _feature_list at the start of each top-level
download by clearing it in begin_manifest before features are registered or
executed. Preserve add_features behavior and ensure subsequent downloads on the
same downloader instance do not reuse features from earlier runs.
In `@src/jmcomic/jm_entity.py`:
- Around line 720-725: Update JmAlbumCommentPage.__init__ so page_number follows
the existing raw_html and raw_data parameters, preferably making it
keyword-only, while preserving current positional argument mappings and
page_number behavior.
In `@src/jmcomic/jm_feature.py`:
- Around line 80-93: Update should_invoke to explicitly validate that
jm_task_context contains a download_type before constructing the expected
after_<type> hook name; when it is missing, raise a clear error identifying the
absent context instead of returning False, while preserving the existing
matching behavior for valid download types.
In `@src/jmcomic/jm_plugin.py`:
- Around line 824-917: The progress state logic is duplicated between
AsyncProgressDownloader and ProgressDownloader. Extract shared fields and
state-only methods (start_progress, before_photo, after_image, after_photo,
after_album, and stop_progress) into a ProgressStateMixin, then have both
downloader classes inherit it; keep async lifecycle methods’ await super()
behavior and use the mixin’s instance helpers without changing progress
behavior.
In `@tests/test_jmcomic/test_jm_client.py`:
- Around line 554-575: Update test_page_number_in_sync_generator and
test_page_number_in_async_generator to invoke do_page_iter through minimal
JmcomicClient and AsyncJmcomicClient instances instead of passing None as self.
Preserve the existing pagination inputs and page-number assertions while
ensuring both tests use valid client objects.
In `@tests/test_jmcomic/test_jm_download_manifest.py`:
- Line 774: In the test cases using self.new_downloader, prefix the unused
unpacked variables photo, album, and option with underscores at the identified
assignments, while preserving the variables that are used by each test.
In `@tests/test_jmcomic/test_jm_download_progress.py`:
- Around line 235-249: Remove the exact rendered line-count assertion in the
test around the UI output checks. Keep the existing content-based assertions for
the JMComic progress panel, or replace the count check with a behavior-based
assertion that remains stable across Rich versions.
In `@tests/test_jmcomic/test_jm_release.py`:
- Around line 117-155: Replace brittle raw-text assertions in
test_workflow_keeps_master_v_prefix_trigger_without_generated_notes,
test_manual_workflow_reads_source_version_from_master,
test_release_workflows_build_before_creating_release, and
test_test_workflows_watch_development_requirements with assertions against
parsed YAML structures and relevant semantic values. In
test_contributing_allows_only_formal_release_prs_to_master, avoid exact prose
matching by asserting stable semantic content or remove the wording-dependent
checks.
In `@tests/test_jmcomic/test_jm_task_context.py`:
- Line 299: Remove create=True from the patches targeting
jmcomic.api.perf_counter in the affected test cases, so unittest.mock validates
that the production module exposes perf_counter and raises an AttributeError if
the import changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30fbcc63-6a13-4a69-8de9-a3d3f9c14c8a
⛔ Files ignored due to path filters (2)
assets/docs/sources/images/download_progress_terminal.pngis excluded by!**/*.pngassets/docs/sources/images/github_actions_tutorial.jpgis excluded by!**/*.jpg
📒 Files selected for processing (49)
.github/CONTRIBUTING.md.github/release.py.github/requirements-dev.txt.github/workflows/benchmark.yml.github/workflows/download.yml.github/workflows/download_dispatch.yml.github/workflows/export_favorites.yml.github/workflows/release.yml.github/workflows/release_auto.yml.github/workflows/test_api.yml.github/workflows/test_html.yml.gitignoreCHANGELOG.mdREADME.mdassets/docs/mkdocs.ymlassets/docs/sources/index.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/11_log_custom.mdassets/docs/sources/tutorial/12_domain_strategy.mdassets/docs/sources/tutorial/13_export_and_feature.mdassets/docs/sources/tutorial/14_async_usage.mdassets/docs/sources/tutorial/15_download_progress.mdassets/docs/sources/tutorial/4_module_custom.mdassets/docs/sources/tutorial/6_plugin.mdassets/docs/sources/tutorial/7_advance.mdassets/docs/sources/tutorial/9_custom_download_dir_name.mdsrc/jmcomic/__init__.pysrc/jmcomic/api.pysrc/jmcomic/cl.pysrc/jmcomic/cli.pysrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_client_interface.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_async_feature.pytests/test_jmcomic/test_jm_cli.pytests/test_jmcomic/test_jm_client.pytests/test_jmcomic/test_jm_download_manifest.pytests/test_jmcomic/test_jm_download_progress.pytests/test_jmcomic/test_jm_feature.pytests/test_jmcomic/test_jm_release.pytests/test_jmcomic/test_jm_task_context.py
💤 Files with no reviewable changes (3)
- assets/docs/sources/tutorial/7_advance.md
- src/jmcomic/cl.py
- assets/docs/sources/tutorial/4_module_custom.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
hect0x7
changed the title
Removed redundant command examples and progress bar installation instructions from README.
hect0x7
changed the title
3c2342d to
741591c
Compare
hect0x7
changed the title
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Release