v2.7.0: 新增 async 异步版全套 API; 支持 html 正则解析失败时自动保存网页代码，方便排查; 新增异步vs同步性能 Benchmark; 优化文档。 (#542)#542
Conversation
📝 WalkthroughWalkthroughVersion 2.7.0 introduces full async support by adding ChangesAsync API Feature (v2.7.0)
Sequence Diagram(s)sequenceDiagram
participant Caller
participant api_py as download_album_async
participant JmAsyncDownloader
participant AsyncJmApiClient
participant AsyncSession
Caller->>api_py: download_album_async(album_id)
api_py->>JmAsyncDownloader: __aenter__()
JmAsyncDownloader->>AsyncJmApiClient: setup()
AsyncJmApiClient->>AsyncSession: _ensure_session()
AsyncJmApiClient->>AsyncJmApiClient: auto_update_domain()
AsyncJmApiClient->>AsyncJmApiClient: ensure_have_cookies()
api_py->>JmAsyncDownloader: download_album(album_id)
JmAsyncDownloader->>AsyncJmApiClient: get_album_detail(album_id)
AsyncJmApiClient-->>JmAsyncDownloader: JmAlbumDetail
loop each photo (asyncio.gather)
JmAsyncDownloader->>AsyncJmApiClient: get_photo_detail(photo_id)
AsyncJmApiClient-->>JmAsyncDownloader: JmPhotoDetail
loop each image (semaphore-gated)
JmAsyncDownloader->>AsyncJmApiClient: get_jm_image(img_url)
AsyncJmApiClient-->>JmAsyncDownloader: JmImageResp bytes
JmAsyncDownloader->>JmAsyncDownloader: ThreadPoolExecutor decode+save
end
end
JmAsyncDownloader-->>api_py: JmAlbumDetail
api_py->>JmAsyncDownloader: __aexit__()
api_py-->>Caller: (album, downloader)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
tests/test_jmcomic/test_jm_async_feature.py (1)
45-48: ⚡ Quick winAvoid hard-coding remote album invoke counts in a live-network symmetry test.
Line 47 ties the test to current server content (
album(438516)chapter count). Keep parity as the primary invariant and use a non-brittle lower-bound/assertion for sync count.Suggested patch
self.assert_sync_async_equal(sync_count, async_count, 'feature.invoke_count (album)') - self.assertEqual(sync_count, 2, 'album(438516) 有 1 章，应触发 2 次') + self.assertGreater(sync_count, 0, 'download_album should trigger Feature at least once')🤖 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_async_feature.py` around lines 45 - 48, The test is hard-coding a specific invoke count expectation (sync_count == 2) that depends on the current state of remote server data for album(438516). Remove or replace the brittle assertEqual assertion for sync_count with a non-brittle check such as assertGreaterEqual to establish a lower bound, while keeping the assert_sync_async_equal call as the primary invariant to ensure sync and async counts match each other regardless of the actual chapter count on the server.
🤖 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/benchmark.yml:
- Line 16: Replace the floating action tag in the actions/checkout action with a
pinned commit SHA to ensure supply-chain security and immutability. Instead of
using `@v4`, pin it to a specific commit SHA (for example,
`@9bb56186c3b09b4f86b1c65136965f03a2c6a963` or whichever is the appropriate commit
for your needs). Additionally, add the parameter persist-credentials: false to
the checkout action configuration to harden checkout credentials by preventing
the automatic persistence of the default GitHub token, reducing the scope of
credentials available to subsequent steps.
In `@src/jmcomic/jm_async_client.py`:
- Around line 497-510: The search() method and categories_filter() method accept
category and sub_category filter parameters but do not forward them to the API
request. Update both methods to include these filter parameters in the params
dictionary sent to the API, and update the cache_key construction to include
these filter values so that filtered and unfiltered queries are cached
separately. If the mobile API does not support these filters, explicitly reject
or document that constraint in the method signatures instead. At the main
search() method location (around line 497-510), add category and sub_category to
both the cache_key tuple and the params dictionary. Apply the same changes to
the categories_filter() method at lines 529-545 for the sub_category parameter.
- Around line 586-593: The add_favorite_album method accepts a folder_id
parameter but fails to include it in the API request payload, causing all
favorite albums to be added to the default folder regardless of the specified
folder_id argument. Modify the data dictionary being passed to self.req_api() in
the add_favorite_album method to include the folder_id value alongside the
existing aid parameter, ensuring the folder_id parameter is actually sent to the
API endpoint.
In `@src/jmcomic/jm_async_downloader.py`:
- Around line 39-45: The image_concurrency and photo_concurrency values are
passed directly to asyncio.Semaphore without validation, which can cause runtime
failures or deadlocks if the config contains invalid values like strings, zero,
or negative numbers. Before constructing the _image_semaphore and
_photo_semaphore objects, add validation logic that converts these values to
integers and rejects non-positive values, similar to how the async client guards
max_clients. This ensures bad configuration is caught immediately rather than
causing the async pipeline to hang or fail unexpectedly.
In `@src/jmcomic/jm_client_interface.py`:
- Line 271: The method call to `save_image_resp()` at line 271 is not preserving
its return value, which breaks the delegated method chain for subclasses that
may return metadata or sentinel values. Capture and return the result from the
`save_image_resp()` call instead of discarding it. Apply the same fix to the
other affected locations (lines 282-287) where `download_image()` or similar
delegated methods are called without preserving their return values.
In `@src/jmcomic/jm_downloader.py`:
- Around line 263-291: The download_by_photo_detail method is decorated with
`@catch_exception` which records failures but then re-raises the exception,
causing the execute_on_condition batch to abort on the first failure instead of
continuing to process remaining items. Remove the `@catch_exception` decorator
from download_by_photo_detail and any similar methods in the related code range
(293-319, likely download_by_image_detail) so that exceptions don't interrupt
batch processing. This will allow execute_on_condition to handle all items and
let the failure aggregation logic collect all failures into a single
PartialDownloadFailedException at the end.
In `@src/jmcomic/jm_entity.py`:
- Around line 224-225: The type annotations for from_photo and query_params
attributes are incorrectly declared as non-optional, but these attributes are
initialized from None and are still checked against None throughout the codebase
(in JmDetailClient.check_photo and AsyncJmcomicClient.check_photo). Keep the
type annotations as Optional to maintain sound type contracts for downstream
type checkers. This issue appears at two locations in src/jmcomic/jm_entity.py:
lines 224-225 (the from_photo and query_params initialization) and lines 323-324
(another similar back-reference initialization). Both should maintain Optional
type annotations until construction guarantees these attributes are always
present.
In `@src/jmcomic/jm_exception.py`:
- Around line 88-90: The file uses PEP 604 union syntax (dict | None) in the
context parameter type annotation which breaks Python 3.9 compatibility at
import time. Add from __future__ import annotations at the very top of the
src/jmcomic/jm_exception.py file before all other imports to enable postponed
annotation evaluation, which allows the newer syntax to work on Python 3.9 and
earlier versions. Alternatively, replace dict | None with Optional[dict] in the
context parameter annotation and ensure Optional is imported from typing if not
already present.
In `@tests/test_jmcomic/__init__.py`:
- Around line 127-129: The event loop set in the setup method (cls._loop via
cls._asyncio.set_event_loop()) needs to be properly cleaned up in the teardown
method around lines 143-149. Before closing the loop with cls._loop.close(),
first clear it as the default event loop by calling
cls._asyncio.set_event_loop(None). This prevents the closed loop from remaining
as the default and leaking into subsequent tests.
In `@tests/test_jmcomic/test_jm_async_api.py`:
- Around line 8-13: The module uses asyncio.run(...) in multiple places but does
not explicitly import asyncio, instead relying on wildcard import leakage from
test_jmcomic. Add an explicit import asyncio statement to the import section at
the top of the file, alongside the other imports from jmcomic, to make the
dependency clear and avoid relying on wildcard imports.
In `@tests/test_jmcomic/test_jm_async_client.py`:
- Line 8: The file test_jmcomic/test_jm_async_client.py uses asyncio calls
throughout the code but does not have an explicit import for the asyncio module,
relying instead on a hidden dependency from the wildcard import `from
test_jmcomic import *`. Add an explicit `import asyncio` statement at the top of
the file alongside the existing wildcard import to directly declare this
dependency and remove the reliance on hidden imports.
- Around line 102-111: The broad except Exception: pass block on line 109
catches all exceptions including assertion failures from line 107, which masks
real parity failures between sync and async behavior. Replace the catch-all
Exception handler with specific exception types that represent network and
ranking fluctuations (such as ConnectionError, TimeoutError, or other
connection-related exceptions), allowing AssertionError from the
assert_sync_async_equal call to propagate and properly fail the test when sync
and async results diverge. This preserves the intent of not blocking on network
issues while ensuring actual parity failures are detected.
In `@tests/test_jmcomic/test_jm_async_custom.py`:
- Around line 26-33: The test allocates an asyncio event loop using
asyncio.new_event_loop() and creates a client with
loop.run_until_complete(opt.new_jm_async_client()) but never closes either
resource, causing resource leaks that can destabilize subsequent tests. After
running all assertions in the test (at lines 26-33, also at 77-80 and 92-97 in
the same file), ensure you close the client and event loop by adding cleanup
code that calls client.close() and loop.close() or by wrapping the client
creation and usage in a try-finally block to guarantee cleanup occurs even if
assertions fail.
In `@usage/benchmark_async_vs_sync.py`:
- Line 281: The print statement contains an f-string prefix on a string literal
that has no variable interpolations or placeholders. Remove the f-prefix from
the string in the print function call that outputs the message about report
generation (starting with \n✅ 评测完成) since it contains no {} expressions that
would require the f-string prefix.
- Around line 87-93: The clean_download_dir function currently only removes
subdirectories within base_dir but does not remove root-level files, which can
leave artifacts that interfere with repeated benchmark runs. Modify the function
to also delete files located directly in base_dir by adding logic after the
existing directory removal loop to iterate through os.listdir(base_dir) again
and remove any items that are files (use os.path.isfile to check) in addition to
the existing subdirectory removal with shutil.rmtree.
- Around line 148-151: The async benchmark uses unbounded fan-out by calling
asyncio.gather with all photos at once, while the sync version executes
check_photo serially, creating an unfair comparison that skews results. Modify
the asyncio.gather call within the loop (after the client.get_album_detail call)
to implement bounded concurrency using a semaphore or a concurrency-limiting
utility like asyncio.Semaphore or a helper function that limits the number of
concurrent check_photo calls. This ensures the async and sync benchmarks run
under similar concurrency constraints for accurate comparison.
---
Nitpick comments:
In `@tests/test_jmcomic/test_jm_async_feature.py`:
- Around line 45-48: The test is hard-coding a specific invoke count expectation
(sync_count == 2) that depends on the current state of remote server data for
album(438516). Remove or replace the brittle assertEqual assertion for
sync_count with a non-brittle check such as assertGreaterEqual to establish a
lower bound, while keeping the assert_sync_async_equal call as the primary
invariant to ensure sync and async counts match each other regardless of the
actual chapter count on the server.
🪄 Autofix (Beta)
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
Run ID: 6b467a01-4278-469a-a1d2-6ee78e2fdace
📒 Files selected for processing (25)
.github/workflows/benchmark.yml.gitignoreREADME.mdassets/docs/sources/roadmap.mdassets/docs/sources/tutorial/13_export_and_feature.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_client_interface.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_exception.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/__init__.pytests/test_jmcomic/test_jm_async_api.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_async_custom.pytests/test_jmcomic/test_jm_async_feature.pytests/test_jmcomic/test_jm_async_plugin.pyusage/benchmark_async_vs_sync.py
💤 Files with no reviewable changes (1)
- assets/docs/sources/tutorial/13_export_and_feature.md
hect0x7
changed the title
hect0x7
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/benchmark.yml (1)
50-55: 💤 Low valueConsider adding explicit missing-file handling to artifact upload.
If the benchmark script fails to generate
PERFORMANCE_REPORT.md, the artifact upload will fail with a default error. Addingif-no-files-found: warnwould make the failure more graceful and align with the error handling in the summary step (lines 41-48).📦 Proposed enhancement
- name: Upload Benchmark Report if: always() uses: actions/upload-artifact@v4 with: name: benchmark-report path: PERFORMANCE_REPORT.md + if-no-files-found: warn🤖 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/benchmark.yml around lines 50 - 55, Add the `if-no-files-found: warn` parameter to the "Upload Benchmark Report" step that uses the `actions/upload-artifact@v4` action. This parameter should be added at the same indentation level as the existing `name` and `path` fields within the `with` block to gracefully handle the case where the PERFORMANCE_REPORT.md file is not generated by the benchmark script, preventing the artifact upload from failing with a hard error and aligning with the error handling patterns used elsewhere in the workflow.
🤖 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 @.github/workflows/benchmark.yml:
- Around line 50-55: Add the `if-no-files-found: warn` parameter to the "Upload
Benchmark Report" step that uses the `actions/upload-artifact@v4` action. This
parameter should be added at the same indentation level as the existing `name`
and `path` fields within the `with` block to gracefully handle the case where
the PERFORMANCE_REPORT.md file is not generated by the benchmark script,
preventing the artifact upload from failing with a hard error and aligning with
the error handling patterns used elsewhere in the workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdeb6366-a1d6-40ef-ad1b-30bd1923ade5
📒 Files selected for processing (2)
.github/workflows/benchmark.ymlsrc/jmcomic/jm_exception.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/jmcomic/jm_exception.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 (2)
tests/test_jmcomic/test_jm_async_feature.py (1)
57-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse an explicit
photo_idfordownload_photoparity checks.Lines 57 and 66 pass
album_idintodownload_photo/download_photo_async. That only works when album/photo IDs happen to coincide, which makes this test fragile.Proposed patch
album_id = '438516' + photo_id = str(self.sync_api_client.get_album_detail(album_id)[0].photo_id) @@ - jmcomic.download_photo(album_id, self.option, extra=SyncPhotoCounter()) + jmcomic.download_photo(photo_id, self.option, extra=SyncPhotoCounter()) @@ - asyncio.run(download_photo_async(album_id, self.option, extra=AsyncPhotoCounter())) + asyncio.run(download_photo_async(photo_id, self.option, extra=AsyncPhotoCounter()))🤖 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_async_feature.py` around lines 57 - 67, The test is passing album_id to both download_photo and download_photo_async, which is fragile because it only works when album_id and photo_id happen to coincide. Define or extract an explicit photo_id value and use it instead of album_id in both the download_photo call on line 57 and the download_photo_async call on line 66 to make the test more robust and clearly test the intended photo ID.usage/benchmark_async_vs_sync.py (1)
115-135:⚠️ Potential issue | 🟡 MinorFix benchmark fairness: sync and async downloaders have asymmetrical lifecycle.
The sync download path creates a new
JmDownloader(option)instance inside theCI_REPEATloop (line 123), paying full initialization and client creation cost three times. The async path creates oneJmAsyncDownloadervia context manager outside the loop, reusing the same client across all three iterations.This produces:
- Sync: Cold-start overhead × 3 (new client each iteration)
- Async: Cold-start overhead × 1, then warm-state reuse × 2 (connection pooling, persisted state)
For fair comparison when
CI_REPEAT=3, both paths should use the same initialization strategy.Suggested fix: Move sync downloader creation outside the loop:
def run_sync_download(option: JmOption, album: JmAlbumDetail, base_dir: str) -> float | None: start = time.time() try: + dler = JmDownloader(option) for rep in range(CI_REPEAT): if rep > 0: clean_download_dir(base_dir) - dler = JmDownloader(option) if LIMIT_IMAGES is not None:Alternatively, recreate the async downloader each iteration to test cold-start parity.
🤖 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 `@usage/benchmark_async_vs_sync.py` around lines 115 - 135, The run_sync_download function creates a new JmDownloader instance inside the CI_REPEAT loop on line 123, paying full initialization cost three times, while the async equivalent reuses a single client instance. To achieve fair benchmark comparison, move the JmDownloader(option) instantiation outside and before the CI_REPEAT loop, so it initializes only once. Keep the clean_download_dir call and the do_filter lambda inside the loop to maintain isolation between iterations while reusing the same downloader client across all repetitions.
♻️ Duplicate comments (1)
tests/test_jmcomic/test_jm_async_client.py (1)
104-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow the broad exception handling in search parity loop.
Line 111 still catches all
Exception, so assertion failures and transient network failures are conflated intoAssertionError(elist)at Line 120. This makes parity regressions harder to diagnose and keeps the original blind-catch risk.Proposed patch
for expected_id, params in cases.items(): try: sync_page = self.sync_api_client.search_site(**params) async_page = self.run_async(self.async_client.search_site(**params)) sync_first_aid = int(sync_page[0][0]) async_first_aid = int(async_page[0][0]) self.assert_sync_async_equal(sync_first_aid, async_first_aid, f'search_params[{expected_id}].first_aid') - except Exception as e: + except (ConnectionError, TimeoutError, OSError) as e: elist.append(e)🤖 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_async_client.py` around lines 104 - 121, The exception handling in the search parity loop is too broad, catching all Exception types which conflates assertion failures with transient network failures and makes regressions harder to diagnose. Replace the catch-all except Exception block with specific exception types that represent transient network failures (such as connection errors, timeouts, etc.). This way assertion failures from assert_sync_async_equal calls will propagate naturally to fail the test with clear diagnostics, while only true network-related transient failures are collected and re-raised as AssertionError at the end.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 `@tests/test_jmcomic/test_jm_async_feature.py`:
- Line 47: In the assertGreater call on line 47 of the test file, replace the
fullwidth comma (，) with a standard ASCII comma (,) in the assertion message
string. The message currently reads 'album(438516) 有 1 章，应至少触发 1 次' and the
fullwidth comma (the one after 章) should be changed to a regular ASCII comma to
satisfy the Ruff RUF001 linting rule.
---
Outside diff comments:
In `@tests/test_jmcomic/test_jm_async_feature.py`:
- Around line 57-67: The test is passing album_id to both download_photo and
download_photo_async, which is fragile because it only works when album_id and
photo_id happen to coincide. Define or extract an explicit photo_id value and
use it instead of album_id in both the download_photo call on line 57 and the
download_photo_async call on line 66 to make the test more robust and clearly
test the intended photo ID.
In `@usage/benchmark_async_vs_sync.py`:
- Around line 115-135: The run_sync_download function creates a new JmDownloader
instance inside the CI_REPEAT loop on line 123, paying full initialization cost
three times, while the async equivalent reuses a single client instance. To
achieve fair benchmark comparison, move the JmDownloader(option) instantiation
outside and before the CI_REPEAT loop, so it initializes only once. Keep the
clean_download_dir call and the do_filter lambda inside the loop to maintain
isolation between iterations while reusing the same downloader client across all
repetitions.
---
Duplicate comments:
In `@tests/test_jmcomic/test_jm_async_client.py`:
- Around line 104-121: The exception handling in the search parity loop is too
broad, catching all Exception types which conflates assertion failures with
transient network failures and makes regressions harder to diagnose. Replace the
catch-all except Exception block with specific exception types that represent
transient network failures (such as connection errors, timeouts, etc.). This way
assertion failures from assert_sync_async_equal calls will propagate naturally
to fail the test with clear diagnostics, while only true network-related
transient failures are collected and re-raised as AssertionError at the end.
🪄 Autofix (Beta)
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
Run ID: 72a1a70e-cd73-4de4-92d9-873ac6d8b873
📒 Files selected for processing (7)
.github/workflows/benchmark.ymlsrc/jmcomic/jm_async_downloader.pytests/test_jmcomic/__init__.pytests/test_jmcomic/test_jm_async_api.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_async_feature.pyusage/benchmark_async_vs_sync.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_jmcomic/test_jm_async_api.py
- tests/test_jmcomic/init.py
- src/jmcomic/jm_async_downloader.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/14_async_usage.md`:
- Around line 22-23: The example code using download_batch_async is incomplete
and missing the required download_api parameter (the first parameter that
specifies which API function to call). Replace this example with the correct
public API methods download_album_async or download_photo_async, passing the
list of IDs as the argument—both of these methods natively support batch
operations and are the documented way to perform batch downloads.
- Around line 55-75: The main function creates an AsyncJmApiClient using `await
op.new_jm_async_client()` but never closes it, leaving the async session and
resources open. Wrap the client creation with an `async with` statement to
ensure automatic cleanup, or alternatively call `await cl.close()` in a finally
block after all client operations are complete to properly release sockets and
file descriptors. Apply this cleanup pattern consistently across all async
client example snippets in the document.
🪄 Autofix (Beta)
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
Run ID: 333fd41d-3707-4e4a-b376-1ddabacffb83
📒 Files selected for processing (11)
README.mdassets/docs/mkdocs.ymlassets/docs/sources/api/client.mdassets/docs/sources/api/download.mdassets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/14_async_usage.mdassets/docs/sources/tutorial/9_custom_download_dir_name.mdassets/readme/README-en.mdassets/readme/README-jp.mdassets/readme/README-kr.md
✅ Files skipped from review due to trivial changes (6)
- assets/docs/mkdocs.yml
- assets/docs/sources/tutorial/0_common_usage.md
- assets/readme/README-jp.md
- assets/docs/sources/api/download.md
- assets/docs/sources/api/client.md
- assets/readme/README-en.md
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/docs/sources/tutorial/9_custom_download_dir_name.md
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)
src/jmcomic/jm_async_downloader.py (1)
40-45:⚠️ Potential issue | 🟡 MinorCheck
is Noneinstead of usingorfor concurrency fallback.An explicit
image_concurrency=0orphoto_concurrency=0is falsy and gets replaced by the option defaults before the<= 0validation runs, silently ignoring invalid caller input. Replace theorfallback with an explicitis Nonecheck.Proposed fix
- image_concurrency = int(image_concurrency or option.download.threading.image) + if image_concurrency is None: + image_concurrency = option.download.threading.image + image_concurrency = int(image_concurrency) if image_concurrency <= 0: raise ValueError(f"image_concurrency must be > 0, got {image_concurrency}") - photo_concurrency = int(photo_concurrency or option.download.threading.photo) + if photo_concurrency is None: + photo_concurrency = option.download.threading.photo + photo_concurrency = int(photo_concurrency) if photo_concurrency <= 0: raise ValueError(f"photo_concurrency must be > 0, got {photo_concurrency}")🤖 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` around lines 40 - 45, The code uses the `or` operator to provide fallback values for image_concurrency and photo_concurrency, which treats 0 as falsy and silently replaces explicit zeros with the default option before the validation runs. Replace the `or` fallback with an explicit `is None` check in both the image_concurrency and photo_concurrency assignments (around line 40 and the photo_concurrency assignment that follows). This ensures that caller-provided values like 0 are preserved and properly caught by the subsequent <= 0 validation checks instead of being silently overridden.
🧹 Nitpick comments (1)
tests/test_jmcomic/test_jm_async_client.py (1)
102-120: ⚡ Quick winImprove exception handling to distinguish parity failures from network errors.
Lines 102-112 catch all exceptions including
AssertionErrorfromassert_sync_async_equal(line 109), mixing assertion failures with network errors inelist. When debugging test failures, this makes it harder to distinguish genuine sync/async parity divergence from transient network issues.Consider catching only specific network-related exceptions (e.g.,
ConnectionError,TimeoutError,requests.RequestException) and allowingAssertionErrorto propagate immediately, or at minimum, separate the two exception types when buildingelist.♻️ Proposed refinement to separate network errors from parity failures
+ network_errors = [] + parity_failures = [] for expected_id, params in cases.items(): try: sync_page = self.sync_api_client.search_site(**params) async_page = self.run_async(self.async_client.search_site(**params)) sync_first_aid = int(sync_page[0][0]) async_first_aid = int(async_page[0][0]) self.assert_sync_async_equal(sync_first_aid, async_first_aid, f'search_params[{expected_id}].first_aid') + except AssertionError as e: + parity_failures.append(e) except Exception as e: - elist.append(e) + network_errors.append(e) - if len(elist) == 0: + if len(parity_failures) > 0: + for e in parity_failures: + print(f'Parity failure: {e}') + raise AssertionError(f'Parity failures: {parity_failures}') + + if len(network_errors) == 0: return - for e in elist: - print(e) + for e in network_errors: + print(f'Network error (expected, skipping): {e}') - raise AssertionError(elist)🤖 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_async_client.py` around lines 102 - 120, The exception handling in the try-except block (lines 104-112) catches all exceptions with a bare `except Exception as e:` clause, which indiscriminately mixes network-related errors with assertion failures from the `assert_sync_async_equal` call on line 109. To improve debugging, modify the exception handling to catch only network-specific exceptions (such as ConnectionError, TimeoutError, or requests.RequestException) in the except clause, allowing AssertionError to propagate immediately for clear test failure reporting. Alternatively, maintain separate exception lists for network errors versus assertion errors so they can be distinguished when diagnosing test failures.
🤖 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/14_async_usage.md`:
- Around line 53-57: The async client example code uses the variable `op`
without defining it, making the snippet non-runnable as written. Either add the
missing assignment to create a default JMOption instance before the async
context manager (e.g., `op = jmcomic.JMOption()`), or inline the option creation
directly by replacing `op.new_jm_async_client()` with
`jmcomic.JMOption().new_jm_async_client()` in the async with statement to make
the code block self-contained and immediately executable.
In `@src/jmcomic/jm_async_client.py`:
- Around line 314-315: The setup() method causes a deadlock during cold start
because req_api() calls setup(), which acquires a non-reentrant class-level lock
and then internally calls req_api('/setting'), which tries to call setup() again
on the same instance. The instance guard `if self._has_setup: return` doesn't
prevent this because `self._has_setup` is only set after releasing the lock.
Additionally, the class-level asyncio.Lock is unsafe for clients across
different event loops. Fix this by either: (1) preventing the '/setting'
endpoint from entering the setup path by adding a skip condition in req_api()
that checks if the request path is '/setting', or (2) making _setup_lock an
instance-scoped reentrant lock (or asyncio.Lock per instance) and setting
self._has_setup = True before calling ensure_have_cookies() so re-entry is
properly guarded. The first approach is simpler; implement whichever aligns with
your codebase design.
---
Outside diff comments:
In `@src/jmcomic/jm_async_downloader.py`:
- Around line 40-45: The code uses the `or` operator to provide fallback values
for image_concurrency and photo_concurrency, which treats 0 as falsy and
silently replaces explicit zeros with the default option before the validation
runs. Replace the `or` fallback with an explicit `is None` check in both the
image_concurrency and photo_concurrency assignments (around line 40 and the
photo_concurrency assignment that follows). This ensures that caller-provided
values like 0 are preserved and properly caught by the subsequent <= 0
validation checks instead of being silently overridden.
---
Nitpick comments:
In `@tests/test_jmcomic/test_jm_async_client.py`:
- Around line 102-120: The exception handling in the try-except block (lines
104-112) catches all exceptions with a bare `except Exception as e:` clause,
which indiscriminately mixes network-related errors with assertion failures from
the `assert_sync_async_equal` call on line 109. To improve debugging, modify the
exception handling to catch only network-specific exceptions (such as
ConnectionError, TimeoutError, or requests.RequestException) in the except
clause, allowing AssertionError to propagate immediately for clear test failure
reporting. Alternatively, maintain separate exception lists for network errors
versus assertion errors so they can be distinguished when diagnosing test
failures.
🪄 Autofix (Beta)
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
Run ID: c99969b4-a000-4931-9407-2c7dc4897f25
📒 Files selected for processing (14)
assets/docs/sources/index.mdassets/docs/sources/tutorial/14_async_usage.mdassets/readme/README-en.mdassets/readme/README-jp.mdassets/readme/README-kr.mdsrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_interface.pysrc/jmcomic/jm_option.pytests/test_jmcomic/__init__.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_async_custom.pytests/test_jmcomic/test_jm_async_feature.pyusage/benchmark_async_vs_sync.py
✅ Files skipped from review due to trivial changes (2)
- assets/docs/sources/index.md
- assets/readme/README-en.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_jmcomic/init.py
- usage/benchmark_async_vs_sync.py
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
usage/benchmark_async_vs_sync.py (1)
113-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClient cache is not explicitly disabled for query benchmarking.
At Line 113-116 you disable download cache only, but query timing uses
option.new_jm_client()(Line 151) andoption.new_jm_async_client()(Line 197). AcrossCI_REPEAT, client-level caching can still affectget_album_detail/check_phototimings and skew benchmark parity.🔧 Suggested fix
def run_sync_query(option: JmOption) -> tuple[float | None, JmAlbumDetail | None]: @@ - client = option.new_jm_client() + client = option.new_jm_client(cache=False) @@ async def run_async_query(option: JmOption) -> tuple[float | None, JmAlbumDetail | None]: @@ - async with option.new_jm_async_client() as client: + async with option.new_jm_async_client(cache=False) as client:Also applies to: 151-151, 197-197
🤖 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 `@usage/benchmark_async_vs_sync.py` around lines 113 - 116, The benchmark disables only download cache but not client-level cache, which can affect timing measurements when calling option.new_jm_client() and option.new_jm_async_client(). To fix this, in addition to disabling the download cache with option.download['cache'] and option.decide_download_cache, you must also explicitly disable the client-level cache configuration (typically through an option setting that controls caching for operations like get_album_detail and check_photo) so that repeated benchmark iterations across CI_REPEAT do not benefit from cached results and provide accurate comparative timings.
♻️ Duplicate comments (2)
tests/test_jmcomic/test_jm_async_client.py (1)
105-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow exception scope so parity logic failures cannot be mislabeled as “network errors”.
except Exceptionstill wraps both remote calls and local assertion-prep logic. If indexing/conversion fails, the case is recorded as a network skip and the test may pass incorrectly.Suggested patch
for expected_id, params in cases.items(): try: sync_page = self.sync_api_client.search_site(**params) async_page = self.run_async(self.async_client.search_site(**params)) - sync_first_aid = int(sync_page[0][0]) - async_first_aid = int(async_page[0][0]) - self.assert_sync_async_equal(sync_first_aid, async_first_aid, - f'search_params[{expected_id}].first_aid') except AssertionError as e: parity_failures.append(e) except Exception as e: network_errors.append(e) + continue + + sync_first_aid = int(sync_page[0][0]) + async_first_aid = int(async_page[0][0]) + self.assert_sync_async_equal( + sync_first_aid, + async_first_aid, + f'search_params[{expected_id}].first_aid' + )🤖 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_async_client.py` around lines 105 - 116, The exception handling is too broad and mislabels data conversion failures as network errors. The except Exception handler catches not only the remote API calls (sync_api_client.search_site and async_client.search_site) but also the subsequent indexing and int() conversion logic. Move the lines that extract and convert sync_first_aid and async_first_aid outside the inner try block so they execute after successful API calls, allowing any IndexError or ValueError from data access to be treated as assertion failures rather than network errors. Keep the API calls within the try block that catches network-related exceptions, while letting data preparation failures propagate naturally to surface parity problems.Source: Linters/SAST tools
assets/docs/sources/tutorial/14_async_usage.md (1)
53-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefine
opin the first async-client snippet.This block still isn’t copy/paste runnable:
opis never assigned beforeasync with. Inlinejmcomic.JmOption.default()or add the missing setup first.💡 Suggested fix
-async with op.new_jm_async_client() as cl: +async with jmcomic.JmOption.default().new_jm_async_client() as cl:🤖 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 `@assets/docs/sources/tutorial/14_async_usage.md` around lines 53 - 57, The code snippet using op.new_jm_async_client() is missing the definition of the op variable, making it not copy-paste runnable. Add a line before the async with block to define op by assigning it jmcomic.JmOption.default(), or inline that call directly in the async with statement where op.new_jm_async_client() is referenced.
🧹 Nitpick comments (1)
usage/benchmark_async_vs_sync.py (1)
84-85: ⚡ Quick winAvoid silent
except Exception: passin memory monitor sampling.At Line 84-85 and Line 94-95, swallowing all exceptions can hide monitor failures and still emit “peak memory” values as if valid. Log once (or narrow exception types) so invalid measurements are detectable.
Also applies to: 94-95
🤖 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 `@usage/benchmark_async_vs_sync.py` around lines 84 - 85, The silent except Exception: pass blocks at lines 84-85 and 94-95 in the memory monitor sampling code hide failures and allow invalid measurements to be reported as valid. Instead of silently swallowing all exceptions, add logging to capture and report the actual errors so measurement failures are detectable. Either log the exception details before passing, or narrow the exception types to only catch specific expected exceptions while letting unexpected errors propagate or be logged appropriately at both locations.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_async_client.py`:
- Around line 314-315: The deadlock occurs because `self._has_setup` is set too
late (at line 719, after the lock is released), so when a nested `setup()` call
happens through the chain `req_api()` → `ensure_have_cookies()` → `setting()` →
nested `req_api()` → nested `setup()`, the flag is still `False` and the nested
`setup()` tries to reacquire the non-reentrant `_setup_lock`. At the anchor site
around line 314-315 where `await self.setup()` is called in `req_api()`, add a
check to skip setup for the `/setting` endpoint specifically (bypass the
recursive call). Alternatively, at the sibling site around line 699-719 where
`self._has_setup = True` is set, move this flag assignment to occur immediately
after acquiring `_setup_lock` instead of at the end, so nested calls see the
flag is already set and skip redundant setup.
---
Outside diff comments:
In `@usage/benchmark_async_vs_sync.py`:
- Around line 113-116: The benchmark disables only download cache but not
client-level cache, which can affect timing measurements when calling
option.new_jm_client() and option.new_jm_async_client(). To fix this, in
addition to disabling the download cache with option.download['cache'] and
option.decide_download_cache, you must also explicitly disable the client-level
cache configuration (typically through an option setting that controls caching
for operations like get_album_detail and check_photo) so that repeated benchmark
iterations across CI_REPEAT do not benefit from cached results and provide
accurate comparative timings.
---
Duplicate comments:
In `@assets/docs/sources/tutorial/14_async_usage.md`:
- Around line 53-57: The code snippet using op.new_jm_async_client() is missing
the definition of the op variable, making it not copy-paste runnable. Add a line
before the async with block to define op by assigning it
jmcomic.JmOption.default(), or inline that call directly in the async with
statement where op.new_jm_async_client() is referenced.
In `@tests/test_jmcomic/test_jm_async_client.py`:
- Around line 105-116: The exception handling is too broad and mislabels data
conversion failures as network errors. The except Exception handler catches not
only the remote API calls (sync_api_client.search_site and
async_client.search_site) but also the subsequent indexing and int() conversion
logic. Move the lines that extract and convert sync_first_aid and
async_first_aid outside the inner try block so they execute after successful API
calls, allowing any IndexError or ValueError from data access to be treated as
assertion failures rather than network errors. Keep the API calls within the try
block that catches network-related exceptions, while letting data preparation
failures propagate naturally to surface parity problems.
---
Nitpick comments:
In `@usage/benchmark_async_vs_sync.py`:
- Around line 84-85: The silent except Exception: pass blocks at lines 84-85 and
94-95 in the memory monitor sampling code hide failures and allow invalid
measurements to be reported as valid. Instead of silently swallowing all
exceptions, add logging to capture and report the actual errors so measurement
failures are detectable. Either log the exception details before passing, or
narrow the exception types to only catch specific expected exceptions while
letting unexpected errors propagate or be logged appropriately at both
locations.
🪄 Autofix (Beta)
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
Run ID: b59631f1-6b08-44e2-a2ed-8c85a1653b6e
📒 Files selected for processing (19)
assets/docs/sources/index.mdassets/docs/sources/tutorial/14_async_usage.mdassets/readme/README-en.mdassets/readme/README-jp.mdassets/readme/README-kr.mdpyproject.tomlsetup.pysrc/jmcomic/api.pysrc/jmcomic/cl.pysrc/jmcomic/cli.pysrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_interface.pysrc/jmcomic/jm_option.pytests/test_jmcomic/__init__.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_async_custom.pytests/test_jmcomic/test_jm_async_feature.pyusage/benchmark_async_vs_sync.py
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- assets/readme/README-en.md
🚧 Files skipped from review as they are similar to previous changes (5)
- assets/docs/sources/index.md
- tests/test_jmcomic/init.py
- assets/readme/README-kr.md
- src/jmcomic/jm_option.py
- src/jmcomic/api.py
1. [jm_async_downloader] or → is None 检查，防止显式传入 0 时被静默替换为默认值 2. [14_async_usage.md] 内联 JmOption.default()，使代码片段自包含 3. [jm_async_client] /setting 路径跳过 setup() 调用，防止 asyncio.Lock 不可重入死锁
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/jmcomic/jm_async_client.py (1)
54-56: ⚖️ Poor tradeoffClass-level
_setup_lockand_has_setup_domain_and_cookiesmay cause cross-instance interference.The lock and flag are class attributes, meaning all
AsyncJmApiClientinstances share them. If multiple independent clients are created (e.g., in different event loops or with different options), the first to completesetup()sets_has_setup_domain_and_cookies = True, and subsequent instances skip domain/cookie initialization entirely—even if they need different configurations.Consider making these instance-level or documenting that only one logical client configuration is supported per process.
🤖 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_client.py` around lines 54 - 56, The _setup_lock and _has_setup_domain_and_cookies attributes in the AsyncJmApiClient class are defined as class-level attributes, causing all instances to share the same lock and flag. This prevents multiple independent client instances from properly initializing with different configurations since the first instance sets the flag and subsequent instances skip setup. Move both _setup_lock and _has_setup_domain_and_cookies from class-level attributes into instance-level attributes by initializing them in the __init__ method of AsyncJmApiClient, ensuring each instance has its own independent lock and setup state.src/jmcomic/jm_async_downloader.py (1)
159-194: 💤 Low value
after_imagecallback runs while holding the semaphore.The
_image_semaphorescope (line 160) includes theafter_imagecallback (line 194). If plugins perform I/O or blocking operations inafter_image, they will hold a concurrency slot and reduce effective parallelism.Consider moving
after_imageoutside the semaphore scope, similar to howafter_photois outside_photo_semaphoreindownload_by_photo_detail.🤖 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` around lines 159 - 194, The after_image callback is currently being awaited inside the _image_semaphore context manager scope, which means the semaphore remains held while the callback executes. This reduces concurrency if the callback performs any blocking operations. Move the await self.after_image call statement outside and after the async with self._image_semaphore block, so the semaphore is released before the callback runs. This aligns the pattern with how after_photo is handled outside the _photo_semaphore scope in the download_by_photo_detail method.
🤖 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_async_client.py`:
- Around line 314-316: The `/setting` endpoint skips the `setup()` call to
prevent deadlock, but since `setup()` is responsible for initializing
`self._session` via `_ensure_session()`, this causes `self._session` to remain
`None` when `_request_with_retry` tries to use it, resulting in an
`AttributeError`. Add an `else` branch to the conditional check at the location
where `if url != '/setting': await self.setup()` appears, and in that `else`
branch call `await self._ensure_session()` to ensure the session is properly
initialized for the `/setting` endpoint without invoking the full `setup()`
method that causes the deadlock.
---
Nitpick comments:
In `@src/jmcomic/jm_async_client.py`:
- Around line 54-56: The _setup_lock and _has_setup_domain_and_cookies
attributes in the AsyncJmApiClient class are defined as class-level attributes,
causing all instances to share the same lock and flag. This prevents multiple
independent client instances from properly initializing with different
configurations since the first instance sets the flag and subsequent instances
skip setup. Move both _setup_lock and _has_setup_domain_and_cookies from
class-level attributes into instance-level attributes by initializing them in
the __init__ method of AsyncJmApiClient, ensuring each instance has its own
independent lock and setup state.
In `@src/jmcomic/jm_async_downloader.py`:
- Around line 159-194: The after_image callback is currently being awaited
inside the _image_semaphore context manager scope, which means the semaphore
remains held while the callback executes. This reduces concurrency if the
callback performs any blocking operations. Move the await self.after_image call
statement outside and after the async with self._image_semaphore block, so the
semaphore is released before the callback runs. This aligns the pattern with how
after_photo is handled outside the _photo_semaphore scope in the
download_by_photo_detail method.
🪄 Autofix (Beta)
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
Run ID: 698c3c91-dc44-4342-83d6-fab02871dcae
📒 Files selected for processing (3)
assets/docs/sources/tutorial/14_async_usage.mdsrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_async_downloader.py
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/docs/sources/tutorial/14_async_usage.md
hect0x7
changed the title
There was a problem hiding this comment.
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/__init__.py (1)
191-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep a full-size assertion when
check_total=False.Line 193 makes
page.totaloptional, but Lines 196-198 still compare only the first five IDs. In thecheck_total=Falsepath, a page that drops or adds tail results will now pass as long as the prefix matches. This weakens the ranking parity test instead of just ignoring an unstabletotalfield.Suggested patch
def assert_search_page_equal(self, sync_page, async_page, check_total=True): """断言两个搜索结果页在结构上一致""" + self.assert_sync_async_equal(len(sync_page), len(async_page), 'page.length') if check_total: self.assert_sync_async_equal(sync_page.total, async_page.total, 'page.total') # 比较前 5 条结果的 album_id sync_ids = [aid for aid, _ in sync_page[:min(5, len(sync_page))]] async_ids = [aid for aid, _ in async_page[:min(5, len(async_page))]]🤖 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/__init__.py` around lines 191 - 198, The assert_search_page_equal method currently limits comparison to only the first 5 item IDs regardless of the check_total parameter value. When check_total is False, the assertion should still fully validate all page contents instead of just the prefix, ensuring that dropped or added tail results are caught. Remove or conditionally apply the min(5, len(...)) limiting in the slicing operations that create sync_ids and async_ids, so that when check_total=False, all items from both sync_page and async_page are compared while still skipping the page.total assertion.
🤖 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.
Outside diff comments:
In `@tests/test_jmcomic/__init__.py`:
- Around line 191-198: The assert_search_page_equal method currently limits
comparison to only the first 5 item IDs regardless of the check_total parameter
value. When check_total is False, the assertion should still fully validate all
page contents instead of just the prefix, ensuring that dropped or added tail
results are caught. Remove or conditionally apply the min(5, len(...)) limiting
in the slicing operations that create sync_ids and async_ids, so that when
check_total=False, all items from both sync_page and async_page are compared
while still skipping the page.total assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 246844e3-8616-4bf5-9dc6-549ee6486047
📒 Files selected for processing (2)
tests/test_jmcomic/__init__.pytests/test_jmcomic/test_jm_async_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_jmcomic/test_jm_async_client.py
hect0x7
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
Release Notes - Version 2.7.0
jmcomic/jmventrypoints to the new CLI and deprecated the old CLI shim.