Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds album-cover download support (JmcomicText.get_album_cover_url + JmImageClient.download_album_cover), a DownloadCoverPlugin, tests and docs updates, a fallback rule parser change, domain_retry_strategy injection into AbstractJmClient, an advanced_retry plugin rename, and a version bump. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Caller
participant IC as JmImageClient
participant JT as JmcomicText
participant Net as HTTP
participant FS as FileSystem
Dev->>IC: download_album_cover(album_id, save_path, size?)
IC->>JT: get_album_cover_url(album_id, image_domain?, size?)
JT-->>IC: cover_url
IC->>Net: GET cover_url (scramble_id=None, decode_image=False)
alt 200 OK
Net-->>IC: image bytes
IC->>FS: write save_path (e.g., cover.png)
FS-->>IC: OK
IC-->>Dev: success
else error
IC-->>Dev: error
end
sequenceDiagram
autonumber
participant C as AbstractJmClient
participant S as domain_retry_strategy (optional)
participant Net as HTTP
C->>Net: request(url)
alt success
Net-->>C: response
else failure
opt strategy provided
C->>S: strategy(url, attempt, error)
S-->>C: next_domain / None
alt next_domain provided
C->>Net: request(url@next_domain)
Net-->>C: response/error
else no suggestion
C->>Net: fallback retry logic
Net-->>C: response/error
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 1
🧹 Nitpick comments (7)
src/jmcomic/jm_toolkit.py (1)
384-387: Non-crypto randomness is fine here; hush the linter or switch API.Domain selection isn’t security‑sensitive. Either keep
random.choiceand add# noqa: S311(as in the diff above), or replace withsecrets.choiceif you prefer zero lint noise.tests/test_jmcomic/test_jm_client.py (1)
336-339: Assert the download and avoid polluting CWD.Use a temp directory and assert the file exists to make the test meaningful and hermetic.
- def test_download_cover(self): - album_id = 123 - self.client.download_album_cover(album_id, f'./{album_id}.jpg') + def test_download_cover(self): + import os, tempfile + album_id = 123 + with tempfile.TemporaryDirectory() as td: + path = os.path.join(td, f'{album_id}.jpg') + self.client.download_album_cover(album_id, path) + self.assertTrue(os.path.exists(path), f'cover not downloaded: {path}')src/jmcomic/jm_client_interface.py (1)
296-303: Expose size passthrough and make import explicit for linters.
- Optional: surface
sizeso callers can request variants like "_3x4".- Lint: star import obscures
JmcomicText; add an explicit import or alias.Apply this diff:
- def download_album_cover(self, album_id, save_path: str): + def download_album_cover(self, album_id, save_path: str, size: str = ''): self.download_image( - img_url=JmcomicText.get_album_cover_url(album_id), + img_url=JmcomicText.get_album_cover_url(album_id, size=size), img_save_path=save_path, scramble_id=None, decode_image=False, )And near the top of the file (outside this hunk), add:
from .jm_toolkit import JmcomicText # explicit symbol to satisfy linters (F405)If you prefer not to change the public signature, ignore the
sizepart and only add the explicit import. Please confirm which route you want.assets/docs/sources/tutorial/0_common_usage.md (2)
52-54: Docs: mention optional size for cover.Add an example that shows requesting a specific cover size to match the API surface.
-# 下载本子封面图，保存为 cover.png （图片后缀可指定为jpg、webp等） -client.download_album_cover('427413', './cover.png') +# 下载本子封面图，保存为 cover.png （图片后缀可指定为jpg、webp等） +client.download_album_cover('427413', './cover.png') +# 指定封面尺寸（例如列表页用的 3x4） +client.download_album_cover('427413', './cover_3x4.png', size='_3x4')
69-71: Prefer helper for cover URL in examples.Use
JmcomicText.get_album_cover_urlinstead of hardcoding the albums path.- random_image_domain = JmModuleConfig.DOMAIN_IMAGE_LIST[0] - client.download_image(f'https://{random_image_domain}/media/albums/416130.jpg', './a.jpg') + client.download_image(JmcomicText.get_album_cover_url('416130'), './a.jpg')src/jmcomic/jm_client_impl.py (2)
14-31: Constructor param added; update docstring and typing.Document
domain_retry_strategyand, if possible, type it as an Optional[Callable]. This improves discoverability and plugin authoring.def __init__(self, postman: Postman, domain_list: List[str], retry_times=0, - domain_retry_strategy=None, + domain_retry_strategy=None, ): @@ - :param retry_times: 重试次数 + :param retry_times: 重试次数 + :param domain_retry_strategy: 自定义域名/重试策略回调，签名： + (client, request, url, is_image, *, domain_index, retry_count, **kwargs) -> ResponseOptionally (outside this hunk), annotate the field:
from typing import Optional, Callable, Any self.domain_retry_strategy: Optional[Callable[..., Any]] = domain_retry_strategy
73-80: Pass retry context into custom strategy.Hand the
domain_indexandretry_countto the strategy; it’s often needed for proper backoff/switching.- if self.domain_retry_strategy: - return self.domain_retry_strategy(self, - request, - url, - is_image, - **kwargs, - ) + if self.domain_retry_strategy: + return self.domain_retry_strategy(self, + request, + url, + is_image, + domain_index=domain_index, + retry_count=retry_count, + **kwargs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
assets/docs/sources/tutorial/0_common_usage.md(2 hunks)src/jmcomic/jm_client_impl.py(2 hunks)src/jmcomic/jm_client_interface.py(1 hunks)src/jmcomic/jm_toolkit.py(1 hunks)tests/test_jmcomic/test_jm_client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_jmcomic/test_jm_client.py (1)
src/jmcomic/jm_client_interface.py (1)
download_album_cover(296-302)
src/jmcomic/jm_client_interface.py (5)
src/jmcomic/jm_entity.py (1)
album_id(363-364)src/jmcomic/jm_toolkit.py (1)
get_album_cover_url(372-388)src/jmcomic/jm_option.py (1)
download_album(489-495)src/jmcomic/jm_downloader.py (3)
download_album(85-88)download_by_image_detail(121-146)download_by_image_detail(328-333)tests/test_jmcomic/test_jm_api.py (1)
test_download_album_by_id(13-18)
src/jmcomic/jm_toolkit.py (2)
src/jmcomic/jm_entity.py (1)
album_id(363-364)src/jmcomic/jm_config.py (1)
JmModuleConfig(84-503)
🪛 Ruff (0.13.1)
src/jmcomic/jm_client_interface.py
298-298: JmcomicText may be undefined, or defined from star imports
(F405)
src/jmcomic/jm_toolkit.py
374-374: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
382-382: Docstring contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF002)
386-386: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
386-386: JmModuleConfig may be undefined, or defined from star imports
(F405)
388-388: JmModuleConfig may be undefined, or defined from star imports
(F405)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/jmcomic/jm_toolkit.py (2)
377-383: Docstring punctuation triggers Ruff (RUF002).Switch fullwidth punctuation to ASCII to appease the linter.
Apply this diff:
- 根据本子id生成封面url + 根据本子 id 生成封面 URL @@ - :param image_domain: 图片cdn域名（可传入裸域或含协议的域名） - :param size: 尺寸后缀，例如搜索列表页会使用 size="_3x4" 的封面图 + :param image_domain: 图片 CDN 域名 (可传入裸域或含协议的域名) + :param size: 尺寸后缀, 例如搜索列表页会使用 size="_3x4" 的封面图
388-389: Optional: guard against trailing slashes in domains.If callers pass image_domain with a trailing slash, format_url currently produces a double slash. Consider normalizing in format_url.
Example (outside this hunk):
@classmethod def format_url(cls, path, domain): ExceptionTool.require_true(isinstance(domain, str) and len(domain) != 0, '域名为空') path = path if path.startswith('/') else '/' + path if domain.startswith(JmModuleConfig.PROT): return f'{domain.rstrip("/")}{path}' return f'{JmModuleConfig.PROT}{domain.rstrip("/")}{path}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/jmcomic/jm_toolkit.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/jmcomic/jm_toolkit.py (1)
src/jmcomic/jm_config.py (1)
JmModuleConfig(84-503)
🪛 Ruff (0.13.1)
src/jmcomic/jm_toolkit.py
373-373: Union may be undefined, or defined from star imports
(F405)
374-374: Optional may be undefined, or defined from star imports
(F405)
381-381: Docstring contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF002)
381-381: Docstring contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF002)
382-382: Docstring contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF002)
386-386: JmModuleConfig may be undefined, or defined from star imports
(F405)
🔇 Additional comments (2)
src/jmcomic/jm_toolkit.py (2)
371-389: Album-cover URL helper: logic LGTM.Broader typing + format_url usage resolve the scheme-duplication concern and handle int/str album IDs correctly.
373-386: Import Optional/Union and JmModuleConfig explicitly.Silences F405 and avoids annotation runtime issues in environments without postponed annotations.
Add these imports near the top of the file:
from typing import Optional, Union from .jm_config import JmModuleConfig#!/bin/bash # Verify explicit imports exist rg -nP 'from\s+typing\s+import\s+.*\b(Optional|Union)\b' src/jmcomic/jm_toolkit.py rg -nP 'from\s+\.jm_config\s+import\s+JmModuleConfig' src/jmcomic/jm_toolkit.py
|
@coderabbitai review new commit |
|
@hect0x7 I'll review the new commit for you. This appears to be a feature addition for downloading album cover images, along with some infrastructure improvements for domain retry strategies. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
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_plugin.py (1)
1311-1328: Register plugin 'download-cover'Add a call to JmModuleConfig.register_plugin(DownloadCoverPlugin) (e.g., immediately after the class in src/jmcomic/jm_plugin.py) — current repo search shows no register_plugin invocations except the method definition and a docs example, so 'download-cover' is not being added to JmModuleConfig.REGISTRY_PLUGIN and will fail to resolve from config.
🧹 Nitpick comments (4)
src/jmcomic/jm_option.py (1)
161-163: Make the fallback observable; remove dead code.Silently treating unknown tokens as f-string text is fine, but log once per token to aid misconfig debugging, and drop the commented raise.
- return cls.parse_f_string_rule - # ExceptionTool.raises(f'不支持的rule配置: "{rule}"') + jm_log('dir_rule.fallback', f'未识别的rule片段"{rule}"，按f-string文本处理。') + return cls.parse_f_string_rulesrc/jmcomic/jm_client_interface.py (1)
296-303: Return the delegate call; importJmcomicTextexplicitly to satisfy Ruff F405.
- Keep semantics aligned with
download_by_image_detailby returning the inner call.- Add an explicit import for
JmcomicTextto avoid F405 from star-imports.- def download_album_cover(self, album_id, save_path: str, size: str = ''): - self.download_image( + def download_album_cover(self, album_id, save_path: str, size: str = ''): + """Download album cover without decoding.""" + return self.download_image( img_url=JmcomicText.get_album_cover_url(album_id, size=size), img_save_path=save_path, scramble_id=None, decode_image=False, )Add near the top of the file (outside this hunk):
from .jm_toolkit import JmcomicText # explicit import to avoid F405assets/docs/sources/option_file_syntax.md (1)
197-205: Clarify overwrite behavior for download-cover.The plugin currently always re-downloads/overwrites the cover. Consider adding a note here (or a
skip_if_existsflag) so users know repeated runs won’t be skipped.src/jmcomic/jm_plugin.py (1)
1311-1328: Harden DownloadCoverPlugin: validate context, optional cache-skip, lint nits.
- Validate that either album or photo is provided and that
dir_ruleexists.- Honor global cache by skipping if the target file exists.
- Rename unused kwargs to
_kwargsto satisfy ARG002.class DownloadCoverPlugin(JmOptionPlugin): plugin_key = 'download-cover' - def invoke(self, - dir_rule: dict, - size='', - photo: JmPhotoDetail = None, - album: JmAlbumDetail = None, - downloader=None, - **kwargs) -> None: - album_id = album.id if album else photo.album_id - save_path = self.decide_filepath( - album, photo, - None, None, None, - dir_rule - ) - downloader.client.download_album_cover(album_id, save_path, size) + def invoke(self, + dir_rule: dict, + size='', + photo: JmPhotoDetail = None, + album: JmAlbumDetail = None, + downloader=None, + **_kwargs) -> None: + self.require_param(album is not None or photo is not None, 'download-cover需在before_album/after_photo上下文运行') + self.require_param(isinstance(dir_rule, dict), '必须提供dir_rule配置（包含base_dir与rule）') + + # 构造保存路径 + save_path = self.decide_filepath(album, photo, None, None, None, dir_rule) + + # 避免重复下载（遵循全局cache设置） + if getattr(self.option.download, 'cache', False) and os.path.exists(save_path): + self.log(f'封面已存在，跳过: {save_path}') + return + + album_id = album.id if album else photo.album_id + downloader.client.download_album_cover(album_id, save_path, size)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
assets/docs/sources/option_file_syntax.md(2 hunks)src/jmcomic/jm_client_interface.py(1 hunks)src/jmcomic/jm_option.py(1 hunks)src/jmcomic/jm_plugin.py(2 hunks)tests/test_jmcomic/test_jm_client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_jmcomic/test_jm_client.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/jmcomic/jm_plugin.py (2)
src/jmcomic/jm_entity.py (6)
JmPhotoDetail(292-451)JmAlbumDetail(454-568)album_id(363-364)id(67-68)id(433-434)id(514-515)src/jmcomic/jm_client_interface.py (1)
download_album_cover(296-302)
src/jmcomic/jm_client_interface.py (1)
src/jmcomic/jm_toolkit.py (1)
get_album_cover_url(372-389)
🪛 Ruff (0.13.1)
src/jmcomic/jm_plugin.py
114-114: Optional may be undefined, or defined from star imports
(F405)
114-114: Optional may be undefined, or defined from star imports
(F405)
114-114: Optional may be undefined, or defined from star imports
(F405)
1317-1317: JmPhotoDetail may be undefined, or defined from star imports
(F405)
1318-1318: JmAlbumDetail may be undefined, or defined from star imports
(F405)
1320-1320: Unused method argument: kwargs
(ARG002)
src/jmcomic/jm_client_interface.py
298-298: JmcomicText may be undefined, or defined from star imports
(F405)
🔇 Additional comments (2)
assets/docs/sources/option_file_syntax.md (1)
113-118: LGTM: default rule and “auto Bd” note match implementation.Doc reflects
split_rule_dslauto-insertingBd. Keep braces quoted in YAML as shown to avoid parser surprises.src/jmcomic/jm_plugin.py (1)
114-116: Good change: Optional filename/suffix/base_dir unlocks flexible callers. Also import typing names explicitly.The type loosening is helpful. To quiet Ruff F405 from star-imports, explicitly import typing names at module top.
Add at the top of this file:
from typing import Optional, Dict, Any, List, Callable
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores