Conversation
…根据 download_album/download_photo 来源自适应; 新增 Feature 教程文档
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Feature system for post-download exports, forwards an ChangesFeature System + Integration
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Downloader
participant Hook
participant Feature
participant Plugin
User->>API: download_album(..., extra=Feature.export_pdf)
API->>Downloader: dler.add_features(extra, "download_album")
API->>Downloader: start download
Hook->>Downloader: after_album(album)
Downloader->>Feature: should_invoke(feature_from="download_album", when="after_album")?
Feature-->>Downloader: True
Downloader->>Feature: invoke(option, feature_from="download_album", when="after_album", album=..., downloader=...)
Feature->>Plugin: option.invoke_plugin(... adapted kwargs ...)
Plugin-->>Feature: export complete
Feature-->>Downloader: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 6
🧹 Nitpick comments (1)
tests/test_jmcomic/test_jm_feature.py (1)
89-112: ⚡ Quick winAdd a batch-ID coverage case for
extraexecution.Current tests validate scalar IDs, but not iterable batch input. A small batch test would guard against
extrabeing skipped indownload_batchdelegation.🤖 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_feature.py` around lines 89 - 112, Add a test that exercises the `extra` Feature when using batch input: create a `MyCounterFeature` (subclassing `Feature`) that increments a counter in `invoke`, then call `jmcomic.download_batch([...], self.option, extra=counter_feature)` with an iterable of IDs and assert the counter increased for each expected `after_photo`/`after_album` invocation; this mirrors the existing `test_download_use_feature` flow (which calls `download_album` and `download_photo`) but targets `download_batch` to ensure `extra` is not skipped when handling iterable inputs.
🤖 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`:
- Around line 88-90: The link fragment for the inline tip doesn't match the
generated heading anchor for the target heading "### 2.5 智能适配规则"; update the
fragment in the tip (the [智能适配规则](#...) link) to match the heading's generated
anchor (include the "2.5" portion, e.g. change to "#2-5-智能适配规则" or the exact
generated anchor your docs tool emits) so the internal link points to the "###
2.5 智能适配规则" heading.
In `@README.md`:
- Line 33: Update the README entry for the tutorial so the link text reflects
the full coverage ("PDF/ZIP/长图"); specifically change the markdown link label
"[教程：下载后转为 PDF / ZIP]" to "[教程：下载后转为 PDF / ZIP / 长图]" (the target URL
./assets/docs/sources/tutorial/13_export_and_feature.md remains the same) so the
README matches the actual tutorial scope.
In `@src/jmcomic/api.py`:
- Around line 68-69: The batch branch in download_album and download_photo
ignores the extra parameter so features in extra never run for iterable IDs;
update the calls to download_batch(...) to forward the extra argument (e.g.,
change download_batch(download_album, jm_album_id, option, downloader) to
download_batch(download_album, jm_album_id, option, downloader, extra)) and do
the same for the corresponding call in download_photo so download_batch receives
and passes extra through to individual downloads.
In `@src/jmcomic/jm_config.py`:
- Around line 597-599: The code currently calls _os.system('') under the
sys.platform == 'win32' branch, which spawns a shell; replace this with direct
Win32 console mode API calls: use GetStdHandle, GetConsoleMode, SetConsoleMode
and enable the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag (0x0004) for the
STD_OUTPUT_HANDLE to turn on ANSI processing. Locate the platform check
(sys.platform == 'win32') and the _os.system('') call in jm_config.py and swap
it for a ctypes-based implementation that retrieves the stdout handle, reads the
current console mode, ORs in ENABLE_VIRTUAL_TERMINAL_PROCESSING, and writes it
back, handling errors/return codes appropriately.
In `@src/jmcomic/jm_downloader.py`:
- Around line 290-298: The registration code in add_features/feature handling
appends unknown `features` types to self._feature_list and defers errors until
hook invocation; validate types at registration time by checking if `features`
is a list, a FeatureChain (inspect FeatureChain._features), or a supported
single feature type (e.g., the expected Feature class/interface) and raise a
clear TypeError for any unsupported `features` value instead of appending it;
update the branch that currently does self._feature_list.append((features,
feature_from)) to perform an explicit isinstance/issubclass check (or
duck-typing validation) and raise with a helpful message referencing the
offending value.
In `@src/jmcomic/jm_feature.py`:
- Around line 96-103: The current __call__ in PluginFeature overwrites
_user_keys with only the latest kwargs keys, losing previously recorded explicit
keys; change it to merge the previous instance's _user_keys with the new kwargs
keys (e.g., new_instance._user_keys = (getattr(self, "_user_keys", set()) |
set(kwargs.keys()))) so earlier explicit parameters are preserved across chained
PluginFeature(...) calls; ensure you reference __call__, PluginFeature,
_user_keys and kwargs when making this change.
---
Nitpick comments:
In `@tests/test_jmcomic/test_jm_feature.py`:
- Around line 89-112: Add a test that exercises the `extra` Feature when using
batch input: create a `MyCounterFeature` (subclassing `Feature`) that increments
a counter in `invoke`, then call `jmcomic.download_batch([...], self.option,
extra=counter_feature)` with an iterable of IDs and assert the counter increased
for each expected `after_photo`/`after_album` invocation; this mirrors the
existing `test_download_use_feature` flow (which calls `download_album` and
`download_photo`) but targets `download_batch` to ensure `extra` is not skipped
when handling iterable inputs.
🪄 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: 397052b3-440e-4769-968d-68868f23af61
📒 Files selected for processing (11)
README.mdassets/docs/mkdocs.ymlassets/docs/sources/index.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/__init__.pysrc/jmcomic/api.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pytests/test_jmcomic/test_jm_feature.py
修复内容： 1. [文档修正] 修正 13_export_and_feature.md 中的失效锚点链接 2. [文档完善] 补全 README.md 中的教程支持范围描述（增加长图） 3. [核心修复] 修复批量下载时没有透传 extra 参数导致 Feature 无法执行的严重 Bug 4. [性能优化] 改用 ctypes 启用 Windows ANSI 控制台支持，移除低效且不安全的 os.system 进程调用 5. [错误捕获] 提前在 jm_downloader.py 的 add_features 方法中对 extra 进行类型校验 6. [认知升级] 更新代码中的 docstring，强化 Feature 作为“基于生命周期和调用来源的动态自适应机制”的定位 未修改内容及原因： 1. jm_feature.py 链式调用丢失 _user_keys 问题 - 经与开发者确认，暂时先不修改此逻辑。
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/api.py (1)
6-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserve
callbackparameter explicitly before forwarding**kwargs.Line 10 accepts
**kwargswhich are forwarded todownload_api()on line 41. However, line 40 already passescallback=callbackto the same function. If a caller passescallback=throughdownload_batch(), Python raisesTypeError: got multiple values for keyword argument 'callback'before the download runs, making this forwarding surface unsafe for that parameter.🔧 Proposed fix
def download_batch(download_api, jm_id_iter: Union[Iterable, Generator], option=None, downloader=None, + callback=None, **kwargs, ) -> Set[__DOWNLOAD_API_RET]: @@ - def callback(*ret): + def collect_result(*ret): result.add(ret) + if callback is not None: + callback(*ret) @@ - apply_each_obj_func=lambda aid: download_api(aid, - option, - downloader, - callback=callback, - **kwargs, - ), + apply_each_obj_func=lambda aid: download_api( + aid, + option, + downloader, + callback=collect_result, + **kwargs, + ), wait_finish=True )🤖 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 6 - 10, The download_batch function currently accepts **kwargs and also passes callback=callback to download_api, which can cause a TypeError if caller supplies callback in **kwargs; update the download_batch signature to explicitly accept callback (e.g., def download_batch(..., callback=None, **kwargs)) and when forwarding to download_api use callback=callback (or ensure you pop 'callback' from kwargs before calling) so there are no duplicate keyword arguments; reference the download_batch function and the download_api call when making this change.
🤖 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`:
- Around line 35-38: Add explicit fence language "text" to the plain-text code
blocks in this markdown so they no longer trigger MD040: replace each
triple-backtick fence (```) around the directory/tree and flow
examples—specifically the blocks containing the "./" tree, the "[章节标题].pdf"
example, the "Feature (基类)" block, and the "api.download_album(...)" flow—so
each opens with ```text; do the same for the other occurrences referenced
(blocks around lines 83-86, 127-131, 140-161) to keep rendering unchanged while
satisfying the linter.
---
Outside diff comments:
In `@src/jmcomic/api.py`:
- Around line 6-10: The download_batch function currently accepts **kwargs and
also passes callback=callback to download_api, which can cause a TypeError if
caller supplies callback in **kwargs; update the download_batch signature to
explicitly accept callback (e.g., def download_batch(..., callback=None,
**kwargs)) and when forwarding to download_api use callback=callback (or ensure
you pop 'callback' from kwargs before calling) so there are no duplicate keyword
arguments; reference the download_batch function and the download_api call when
making this change.
🪄 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: 5e060789-bafd-4c19-9756-e717a2c75a29
📒 Files selected for processing (6)
README.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/api.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_feature.py
- ZipPlugin: level 默认值改为 None，根据上下文自动推导（有 album 则合并，只有 photo 则单章） - Feature: export_zip 不再传递 level，_adapt_kwargs 移除 level 适配逻辑 - JmOption: compatible_with_old_versions 中新增 _migrate_zip_level，自动将旧 yaml 中的 level 配置等价迁移到正确的钩子位置 - 文档: 更新 option_file_syntax.md 和 13_export_and_feature.md 中 level 相关说明
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/docs/sources/option_file_syntax.md (1)
237-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
after_albumZIP examples still use photo-scoped placeholders.This section is configuring
zipunderafter_album, butfilename_rule: Ptitleand thedir_ruleexample withPid/Ptitleboth require aphotocontext. With the new hook-driven behavior there is no per-photo naming context here, so these examples now contradict the added guidance on Lines 239-241 and will break if users copy them verbatim. UseAxxxplaceholders in this section, or move the photo-scoped examples underafter_photo.🔧 Proposed fix
- filename_rule: Ptitle # 压缩文件的命名规则 + filename_rule: Atitle # 压缩文件的命名规则 ... - # rule: 'Bd / {Atitle} / [{Pid}]-{Ptitle}.zip' # 设置压缩文件夹规则，中间Atitle表示创建一层文件夹，名称是本子标题。[{Pid}]-{Ptitle}.zip 表示压缩文件的命名规则(需显式写出后缀名) - # 使用此方法指定压缩包存储路径则无需和所在钩子对应 + # rule: 'Bd / {Atitle} / [{Aid}]-{Atitle}.zip' # 设置压缩文件夹规则，中间Atitle表示创建一层文件夹，名称是本子标题。[{Aid}]-{Atitle}.zip 表示压缩文件的命名规则(需显式写出后缀名) + # 使用此方法指定压缩包存储路径时，DSL 里的 A/P 变量仍需和所在钩子对应🤖 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/option_file_syntax.md` around lines 237 - 253, The examples under the after_album section use photo-scoped placeholders (filename_rule: Ptitle and dir_rule examples with {Pid}/{Ptitle}) which are invalid in album scope; update those examples to use album-scoped placeholders (Axxx) instead or move the photo-scoped examples into the after_photo section; specifically, change filename_rule Ptitle to an Axxx variant (e.g., Atitle/Aid), update the dir_rule example to use {Atitle}/{Aid} and an album-level filename pattern, and ensure the comments mention that filename_rule, zip_dir and suffix are ignored when dir_rule is present.
🤖 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`:
- Around line 56-62: The example using download_album passes
filename_rule='Ptitle' which PluginFeature.__call__ treats as user-owned and
will be evaluated without a photo context in album scope, causing a runtime
error; update the snippet to use an album-scoped rule (e.g.,
filename_rule='Atitle') or move this example into download_photo so the Pxxx
rule has a photo context, referencing the download_album call and
Feature.export_pdf usage to locate the code to change.
- Around line 134-166: The docs incorrectly state that
PluginFeature/_adapt_kwargs adapt the `level`; update the prose and sequence
diagram to reflect that `level` is now inferred inside ZipPlugin.invoke() and
that PluginFeature._adapt_kwargs() only rewrites `filename_rule` (not `level`),
and adjust the example flow where `_adapt_kwargs('download_album')` is shown to
only remap filename prefixes (A/P) while `ZipPlugin.invoke()` determines
`level`; mention that user-provided params are still preserved and keep
references to PluginFeature, _adapt_kwargs, ZipPlugin.invoke, should_invoke,
`filename_rule`, and `level` so readers know where to look in the code.
In `@src/jmcomic/jm_option.py`:
- Around line 364-377: The migration currently only moves zip with level=photo
from after_album→after_photo but fails to handle after_photo entries with
level='album'; update the migration in jm_option.py (the block handling
group/level, plugin_list, plugins, pinfo, i, and jm_log) to detect when group ==
'after_photo' and level == 'album' and perform the opposite migration: append
pinfo to plugins.setdefault('after_album', []), remove the entry from
plugin_list (plugin_list.pop(i)), emit a jm_log message describing the migration
to after_album, and avoid simply deleting the level; ensure the index i is only
incremented when you do not pop the current item so loop behavior remains
correct.
---
Outside diff comments:
In `@assets/docs/sources/option_file_syntax.md`:
- Around line 237-253: The examples under the after_album section use
photo-scoped placeholders (filename_rule: Ptitle and dir_rule examples with
{Pid}/{Ptitle}) which are invalid in album scope; update those examples to use
album-scoped placeholders (Axxx) instead or move the photo-scoped examples into
the after_photo section; specifically, change filename_rule Ptitle to an Axxx
variant (e.g., Atitle/Aid), update the dir_rule example to use {Atitle}/{Aid}
and an album-level filename pattern, and ensure the comments mention that
filename_rule, zip_dir and suffix are ignored when dir_rule is present.
🪄 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: 7bf0bd43-acd7-4ed9-b8a8-58c95eb5ea48
📒 Files selected for processing (5)
assets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.py
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/jmcomic/jm_feature.py (1)
98-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTypo
_user_0keyssilently breaks the explicit-kwargs tracking; also merge prior keys.Two issues stacked here:
new_instance._user_0keys = ...is a typo for_user_keys(note the stray0). The attribute set on__init__(self._user_keys: set = set()) is never overwritten, so_user_keysalways stays empty and any “explicit user kwargs” logic that relies on it (now or in future) is dead.- Even after fixing the typo, this assignment overwrites previously recorded user keys when
PluginFeature(...)is chained, which the previous review already raised. Merge withself._user_keys.🔧 Proposed fix
def __call__(self, **kwargs): """带自定义参数，返回新实例（继承默认参数）""" new_kwargs = self.kwargs.copy() new_kwargs.update(kwargs) new_instance = PluginFeature(self.plugin_key, **new_kwargs) - # 记录用户显式传入的参数名，这些参数不被动态适配 - new_instance._user_0keys = set(kwargs.keys()) + # 记录用户显式传入的参数名，这些参数不被动态适配 + new_instance._user_keys = set(self._user_keys) | set(kwargs.keys()) return new_instance🤖 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 98 - 105, In PluginFeature.__call__, fix the typo that sets new_instance._user_0keys (should be _user_keys) and merge existing keys instead of overwriting them: compute the union of self._user_keys and the keys from kwargs and assign that to new_instance._user_keys so chained calls preserve prior explicit-kwargs tracking; keep the rest of the new_kwargs logic the same.assets/docs/sources/tutorial/13_export_and_feature.md (2)
73-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
filename_rule='Ptitle'in adownload_albumexample will fail at runtime.Current
PluginFeature._adapt_plugin_kwargsonly fills a default viasetdefault, so the explicitPtitleis forwarded unchanged intoimg2pdfinvoked from theafter_albumhook — nophotois in scope there, so the rule resolution will blow up. Use anAxxxrule here, or move the snippet todownload_photo.🔧 Proposed fix
download_album('123', option, extra=Feature.export_pdf( # 下面是自定义参数 pdf_dir='D:/my_pdfs', # PDF 保存到 D:/my_pdfs 文件夹 - filename_rule='Ptitle', # 用章节标题作为文件名 + filename_rule='Atitle', # 用本子标题作为文件名 delete_original_file=True, # 合并完 PDF 后删除原图 ))🤖 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/13_export_and_feature.md` around lines 73 - 80, The example uses filename_rule='Ptitle' inside download_album which will fail because PluginFeature._adapt_plugin_kwargs only setdefaults and the after_album hook calls img2pdf without a photo in scope; change the example to use an album-level rule (e.g. an Axxx rule) or move the snippet into download_photo, and/or update PluginFeature._adapt_plugin_kwargs to resolve/translate filename_rule for album contexts before forwarding to after_album; specifically adjust the example referencing Feature.export_pdf and filename_rule or alter PluginFeature._adapt_plugin_kwargs so it resolves Ptitle when used in after_album or rejects/rewrites it to an A-prefixed rule.
177-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFlow diagram and bullet still claim
levelis adapted by_adapt_kwargs.
levelis no longer adapted byPluginFeature._adapt_plugin_kwargs(it was migrated out, andZipPlugininfers granularity at runtime). Also,_adapt_plugin_kwargsonly sets a defaultfilename_rule; it does not actually swapPtitle→Atitle/Pid→Aidas the comment in the diagram (# Atitle 不变, Ptitle→Atitle, Pid→Aid) suggests. Either align the prose to “fills a defaultfilename_ruleif not provided; ZIP packaging level is inferred by the plugin” or add the prefix-swap logic in code (see related comment onjm_feature.py).src/jmcomic/jm_option.py (1)
364-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
after_photo + level: albumis still not migrated.The else-branch on Line 376 just removes
leveland leaves the plugin inafter_photo, silently turning a legacyafter_photo: [{plugin: zip, kwargs: {level: album}}]config from “one merged archive per album” into “one archive per photo”. Mirror theafter_album → after_photomigration so the inverse case rehomes toafter_albuminstead of changing semantics.🔧 Proposed fix
if group == 'after_album' and level != 'album': # after_album + level=photo → 等价迁移到 after_photo plugins.setdefault('after_photo', []).append(pinfo) plugin_list.pop(i) jm_log('option.migrate', ...) + elif group == 'after_photo' and level == 'album': + # after_photo + level=album → 等价迁移到 after_album + plugins.setdefault('after_album', []).append(pinfo) + plugin_list.pop(i) + jm_log('option.migrate', + f'[zip 插件迁移] level 参数已过时，已自动将 after_photo 下的 ' + f'zip(level={level!r}) 等价迁移到 after_album。') else: if level != 'photo': jm_log('option.migrate', ...) i += 1🤖 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_option.py` around lines 364 - 380, The migration currently only moves plugins when group == 'after_album' and level != 'album', but it fails to handle the inverse legacy case where group == 'after_photo' and level == 'album'; instead it leaves the plugin in plugin_list and logs removing the level which changes semantics. Update the logic in jm_option.py around the block using variables group, level, plugins, plugin_list, pinfo and i so that when group == 'after_photo' and level == 'album' you rehome the plugin to plugins.setdefault('after_album', []).append(pinfo), remove it from plugin_list (plugin_list.pop(i)), emit a jm_log message mirroring the existing '[zip 插件迁移]' guidance (suggest the equivalent after_album config) and do not increment i for that branch; keep the existing behavior for other cases (including logging and i += 1) intact.
🧹 Nitpick comments (1)
src/jmcomic/jm_feature.py (1)
75-96: 💤 Low value
PluginFeature.kwargsis shared mutable state between instances created via__call__.
__init__keeps a reference to the caller-suppliedkwargsdict (self.kwargs = kwargs). For the predefined module-level instances on Lines 170-172 (Feature.export_pdfetc.), the dict is the literal passed in at module import.__call__doesself.kwargs.copy()so derived instances are safe, but anyone mutatingFeature.export_pdf.kwargsdirectly (or via_adapt_plugin_kwargsif it stopped copying) would scribble on the singleton. Recommend defensive-copying in__init__and usingMappingProxyTypeif you want immutability.🔧 Suggested hardening
def __init__(self, plugin_key, **kwargs): self.plugin_key = plugin_key - self.kwargs = kwargs + self.kwargs = dict(kwargs) # 用户通过 __call__ 显式传入的参数名，这些参数不会被 _adapt_kwargs 动态适配 self._user_keys: set = set()🤖 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 75 - 96, PluginFeature currently stores a direct reference to the caller-supplied kwargs in PluginFeature.__init__ (self.kwargs = kwargs), which creates shared mutable state for module-level feature singletons like Feature.export_pdf and can be mutated by callers or helpers such as _adapt_plugin_kwargs; change __init__ to defensively copy the mapping (e.g., use kwargs.copy()) or wrap it in an immutable MappingProxyType to prevent external mutations, and ensure the existing __call__ and _adapt_plugin_kwargs behavior still operate on copies rather than the original stored dict.
🤖 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`:
- Around line 55-58: The OR-syntax example uses a fullwidth vertical bar (｜)
which is not Python's bitwise-or operator; update the example in the
download_album call to use the ASCII pipe character so it reads
extra=Feature.export_pdf | Feature.export_zip (refer to the download_album(...)
example and the symbols Feature.export_pdf and Feature.export_zip).
In `@src/jmcomic/jm_feature.py`:
- Around line 19-21: Remove the problematic unused import "LiteralString" from
the top of src/jmcomic/jm_feature.py (the line "from typing import
LiteralString") because it breaks Python 3.9/3.10 CI; simply delete that import
so the file only imports the plugin via "from .jm_plugin import *". If you later
actually need LiteralString, import it from typing_extensions and add a
version/gating check instead of importing it directly from typing.
- Around line 125-132: _adapt_plugin_kwargs currently forces full filename_rule
strings and ignores user overrides; update it to use bare rule defaults (e.g.,
'Atitle' for albums, 'Ptitle' for photos) and perform prefix swapping only for
keys not in self._user_keys: if feature_from == 'download_album' set default
filename_rule to 'Atitle' (and map any default rules by replacing leading
'P'→'A'), if feature_from == 'download_photo' set default to 'Ptitle' (and
replace leading 'A'→'P'); do not modify filename_rule (or any other key) when
that key exists in self._user_keys, and continue to base defaults on
self.kwargs.copy().
In `@src/jmcomic/jm_option.py`:
- Around line 378-379: In jm_option.py remove the unnecessary f-string prefix in
the jm_log call: replace jm_log('option.migrate', f'[zip 插件迁移] level
参数已过时，你可以直接删除该参数，不会有任何影响') with a plain string literal (jm_log('option.migrate',
'[zip 插件迁移] level 参数已过时，你可以直接删除该参数，不会有任何影响')) so the f prefix is dropped and
Ruff F541 is avoided.
In `@tests/test_jmcomic/test_jm_feature.py`:
- Around line 63-85: The test references an undefined symbol "when" and asserts
filename_rule values that don't match PluginFeature._adapt_plugin_kwargs
behavior; define or import a valid when value (e.g., when='after_album' or
import the constant used by jm_feature) before calling
Feature.export_pdf/_adapt_plugin_kwargs and then update the assertions to match
the current implementation's outputs (e.g., expect '[JM{Aid}]{Atitle}' or
'[JM{Pid}]{Ptitle}' etc) OR alternatively change
jm_feature.PluginFeature._adapt_plugin_kwargs to return the bare adapted tokens
(like 'Atitle'/'Ptitle'/'Aid') if you prefer to keep the original
assertions—pick one approach and make the change consistently for
Feature.export_pdf, Feature.export_zip, Feature.export_long_img and the custom
Feature.export_zip(filename_rule=...) test.
---
Duplicate comments:
In `@assets/docs/sources/tutorial/13_export_and_feature.md`:
- Around line 73-80: The example uses filename_rule='Ptitle' inside
download_album which will fail because PluginFeature._adapt_plugin_kwargs only
setdefaults and the after_album hook calls img2pdf without a photo in scope;
change the example to use an album-level rule (e.g. an Axxx rule) or move the
snippet into download_photo, and/or update PluginFeature._adapt_plugin_kwargs to
resolve/translate filename_rule for album contexts before forwarding to
after_album; specifically adjust the example referencing Feature.export_pdf and
filename_rule or alter PluginFeature._adapt_plugin_kwargs so it resolves Ptitle
when used in after_album or rejects/rewrites it to an A-prefixed rule.
In `@src/jmcomic/jm_feature.py`:
- Around line 98-105: In PluginFeature.__call__, fix the typo that sets
new_instance._user_0keys (should be _user_keys) and merge existing keys instead
of overwriting them: compute the union of self._user_keys and the keys from
kwargs and assign that to new_instance._user_keys so chained calls preserve
prior explicit-kwargs tracking; keep the rest of the new_kwargs logic the same.
In `@src/jmcomic/jm_option.py`:
- Around line 364-380: The migration currently only moves plugins when group ==
'after_album' and level != 'album', but it fails to handle the inverse legacy
case where group == 'after_photo' and level == 'album'; instead it leaves the
plugin in plugin_list and logs removing the level which changes semantics.
Update the logic in jm_option.py around the block using variables group, level,
plugins, plugin_list, pinfo and i so that when group == 'after_photo' and level
== 'album' you rehome the plugin to plugins.setdefault('after_album',
[]).append(pinfo), remove it from plugin_list (plugin_list.pop(i)), emit a
jm_log message mirroring the existing '[zip 插件迁移]' guidance (suggest the
equivalent after_album config) and do not increment i for that branch; keep the
existing behavior for other cases (including logging and i += 1) intact.
---
Nitpick comments:
In `@src/jmcomic/jm_feature.py`:
- Around line 75-96: PluginFeature currently stores a direct reference to the
caller-supplied kwargs in PluginFeature.__init__ (self.kwargs = kwargs), which
creates shared mutable state for module-level feature singletons like
Feature.export_pdf and can be mutated by callers or helpers such as
_adapt_plugin_kwargs; change __init__ to defensively copy the mapping (e.g., use
kwargs.copy()) or wrap it in an immutable MappingProxyType to prevent external
mutations, and ensure the existing __call__ and _adapt_plugin_kwargs behavior
still operate on copies rather than the original stored dict.
🪄 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: 2ac8b097-2eec-408c-9b14-09650ab81b2a
📒 Files selected for processing (6)
assets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pytests/test_jmcomic/test_jm_feature.py
Owner
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
1 similar comment
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/jmcomic/jm_feature.py (1)
94-99: 💤 Low valueUse
type(self)to keep__call__subclass-safe.
PluginFeature.__call__hardcodes the base class, so any subclass loses its identity when re-parameterized viainstance(...). Switching totype(self)(...)keeps the concrete class.♻️ Proposed refactor
def __call__(self, **kwargs): """带自定义参数，返回新实例（继承默认参数）""" new_kwargs = self.kwargs.copy() new_kwargs.update(kwargs) - new_instance = PluginFeature(self.plugin_key, **new_kwargs) + new_instance = type(self)(self.plugin_key, **new_kwargs) return new_instance🤖 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 94 - 99, The __call__ implementation in PluginFeature currently constructs a new instance with the base class name (PluginFeature(...)) which breaks subclass identity when called on subclasses; change it to instantiate using the runtime class (type(self)(self.plugin_key, **new_kwargs)) so subclasses remain the same type when re-parameterized—update the __call__ method (referenced as PluginFeature.__call__) to use type(self) instead of PluginFeature.
🤖 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 `@src/jmcomic/jm_feature.py`:
- Around line 94-99: The __call__ implementation in PluginFeature currently
constructs a new instance with the base class name (PluginFeature(...)) which
breaks subclass identity when called on subclasses; change it to instantiate
using the runtime class (type(self)(self.plugin_key, **new_kwargs)) so
subclasses remain the same type when re-parameterized—update the __call__ method
(referenced as PluginFeature.__call__) to use type(self) instead of
PluginFeature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0274b4a1-a290-4b05-a97b-7c78268b98ac
📒 Files selected for processing (6)
assets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pytests/test_jmcomic/test_jm_feature.py
1. [核心] 在 JmDownloader 中实现 Feature 异常隔离与详细日志记录，确保单个 Feature 失败不影响下载流程。 2. [优化] 优化 PluginFeature：使用 type(self) 确保子类化安全，使用 dict(kwargs) 避免可变参数污染。 3. [测试] 补全 download_batch 对 extra 参数的传播测试，并新增 Album 模式下使用 Photo 规则的负面测试。 4. [文档] 修正教程文档中的 filename_rule 示例及相关错误，精简代码注释。 5. [其他] 开启 workflow 下载配置的美观打印日志。
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
assets/docs/sources/tutorial/13_export_and_feature.md (2)
157-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
PluginFeatureno longer adaptslevel— docs are inaccurate.
_adapt_plugin_kwargsonly callssetdefaultonfilename_rule; it does not touchlevel. Both Line 157 and Line 189 still claimlevelis dynamically adapted byPluginFeature, which is factually wrong.📝 Proposed fix
-- **PluginFeature**：Feature 的子类，专门封装 jmcomic 插件。除了调用插件之外，还会根据调用来源动态适配 `filename_rule`、`level` 等参数。 +- **PluginFeature**：Feature 的子类，专门封装 jmcomic 插件。除了调用插件之外，还会根据调用来源动态适配 `filename_rule` 参数；ZIP 的打包粒度由插件在运行时根据上下文自动推导。-- **参数自适应**：`PluginFeature` 的 `filename_rule` 前缀（A/P）和 `level`（album/photo）会根据来源动态适配。用户显式传入的参数不会被覆盖。 +- **参数自适应**：`PluginFeature` 的 `filename_rule` 前缀（A/P）会根据来源动态适配；ZIP 的打包粒度由插件根据上下文自动推导。用户显式传入的参数不会被覆盖。Also applies to: 189-189
🤖 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/13_export_and_feature.md` at line 157, The docs incorrectly state that PluginFeature adapts the `level`; update the text in the PluginFeature description and the line near `level` (both occurrences referenced) to reflect actual behavior: `_adapt_plugin_kwargs` only calls `setdefault` on `filename_rule` and does not modify `level`. Replace or reword sentences claiming dynamic adaptation of `level` to say that only `filename_rule` is adapted (and `level` remains unchanged/passed-through), and reference the `_adapt_plugin_kwargs`, `PluginFeature`, and `filename_rule` symbols so readers can correlate behavior with implementation.
42-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFive plain code fences still lack a language identifier (MD040).
The fence blocks at Lines 42, 62, 106, 150, and 163 were flagged in a prior review and reportedly fixed in commit 09233da, but they are still plain
```in the current file. Addingtext(orpythonfor the flow diagram) eliminates the linter warnings without changing rendering.📝 Proposed fix (apply to each of the five locations)
-``` +```text ./ ├── [JM123]本子标题.pdf ← ...Apply the same `text` language tag to the fences at Lines 62, 106, 150, and 163. </details> Also applies to: 62-62, 106-106, 150-150, 163-163 <details> <summary>🤖 Prompt for AI Agents</summary>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/13_export_and_feature.mdat line 42, Update the
five plain triple-backtick fences flagged by the linter so they include a
language identifier: change the ASCII tree blocks (the fence containing "./" and
the tree listing like "├── [JM123]本子标题.pdf") to usetext, and change the flow diagram fence to usepython (or another appropriate language), applying the
same ```text tag to the remaining plain fences; this removes the MD040 warnings
while preserving rendering.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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:
- Around line 73-93: Close the open
python code fence after the first example (the block that uses download_album(..., extra=Feature.export_pdf(...))) so the blockquote starting with "> 💡 小白必读" is rendered as Markdown text, then reopen a newpython fence before "示例 2" so the combo example (using Feature.export_zip
and Feature.export_long_img and download_album) is rendered as a separate code
block; ensure the fences surround only the intended code and that
download_album, Feature.export_pdf, Feature.export_zip, and
Feature.export_long_img remain unchanged.In
@src/jmcomic/jm_downloader.py:
- Around line 311-314: The try/except around feature.invoke currently catches
BaseException (which swallows KeyboardInterrupt/SystemExit); change the handler
to catch Exception instead so interrupts aren't swallowed: update the try block
that calls feature.invoke(self.option, feature_from=feature_from, when=when,
**kwargs) and modify the except BaseException as e to except Exception as e,
keeping the jm_log('downloader.feature.exception', ...) logging behavior intact.
Duplicate comments:
In@assets/docs/sources/tutorial/13_export_and_feature.md:
- Line 157: The docs incorrectly state that PluginFeature adapts the
level;
update the text in the PluginFeature description and the line nearlevel(both
occurrences referenced) to reflect actual behavior:_adapt_plugin_kwargsonly
callssetdefaultonfilename_ruleand does not modifylevel. Replace or
reword sentences claiming dynamic adaptation oflevelto say that only
filename_ruleis adapted (andlevelremains unchanged/passed-through), and
reference the_adapt_plugin_kwargs,PluginFeature, andfilename_rule
symbols so readers can correlate behavior with implementation.- Line 42: Update the five plain triple-backtick fences flagged by the linter so
they include a language identifier: change the ASCII tree blocks (the fence
containing "./" and the tree listing like "├── [JM123]本子标题.pdf") to usetext, and change the flow diagram fence to usepython (or another appropriate
language), applying the same ```text tag to the remaining plain fences; this
removes the MD040 warnings while preserving rendering.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `c71326bd-4e7a-4014-b97f-10a0e1b82b0c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4cec8920f301eb8f949052b107be2cb0f5f51add and 3f8246dc3500ab2343e0cd61b2b422f45ae0dbc6. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `assets/docs/sources/tutorial/13_export_and_feature.md` * `assets/option/option_workflow_download.yml` * `src/jmcomic/jm_downloader.py` * `src/jmcomic/jm_feature.py` * `tests/test_jmcomic/test_jm_feature.py` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * assets/option/option_workflow_download.yml </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/jmcomic/jm_downloader.py (2)
294-296: ⚡ Quick winUse the public
_to_list()method instead of directly accessing the private_featuresattribute.Direct access to
FeatureChain._featurescouples to a private implementation detail. The class already exposes a public method_to_list()to safely iterate over features.♻️ Proposed refactor
elif isinstance(features, FeatureChain): - for f in features._features: + for f in features._to_list(): self._feature_list.append((f, feature_from))🤖 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 294 - 296, The code in the branch that handles FeatureChain directly iterates features._features (private attribute); change it to use the public FeatureChain._to_list() method instead: in the elif isinstance(features, FeatureChain) branch call features._to_list() and iterate that result, appending each (feature, feature_from) to self._feature_list (preserving existing behavior but removing direct access to the private _features attribute).
228-256: 🏗️ Heavy liftRemove the CI pipeline failure claim — it lacks evidence and the new feature dispatch cannot affect the test.
The test
test_partial_exceptiondoes not register any features viaadd_features, so_feature_listremains empty. The_invoke_features_forcall at line 256 iterates over an empty list and has no impact. Additionally,_invoke_features_forinternally catches and logs exceptions (lines 310–311) without re-raising, so even with registered features, this code path cannot propagate exceptions.Two minor concerns remain:
- Line 85:
_feature_list: listshould use a generic type hint:_feature_list: list[tuple]or similar.- Line 295: Accessing the private
_featuresattribute ofFeatureChaindirectly creates tight coupling to an implementation detail. Consider exposing a public method or iterator onFeatureChaininstead.🤖 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 228 - 256, The PR comment claiming a CI pipeline failure should be removed (it's unsupported because test_partial_exception doesn't register features and _invoke_features_for swallows exceptions); update the type annotation for _feature_list to use a generic e.g. list[tuple] (or a more specific tuple type) and stop directly accessing FeatureChain._features by adding/using a public accessor/iterator on FeatureChain (e.g., FeatureChain.features() or __iter__) and replace any direct _features access with that; ensure calls to _invoke_features_for (used in after_album/after_photo) remain but rely on the public FeatureChain API instead of private attributes.
🤖 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 `@src/jmcomic/jm_downloader.py`:
- Around line 294-296: The code in the branch that handles FeatureChain directly
iterates features._features (private attribute); change it to use the public
FeatureChain._to_list() method instead: in the elif isinstance(features,
FeatureChain) branch call features._to_list() and iterate that result, appending
each (feature, feature_from) to self._feature_list (preserving existing behavior
but removing direct access to the private _features attribute).
- Around line 228-256: The PR comment claiming a CI pipeline failure should be
removed (it's unsupported because test_partial_exception doesn't register
features and _invoke_features_for swallows exceptions); update the type
annotation for _feature_list to use a generic e.g. list[tuple] (or a more
specific tuple type) and stop directly accessing FeatureChain._features by
adding/using a public accessor/iterator on FeatureChain (e.g.,
FeatureChain.features() or __iter__) and replace any direct _features access
with that; ensure calls to _invoke_features_for (used in
after_album/after_photo) remain but rely on the public FeatureChain API instead
of private attributes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 160b10af-aafd-43fa-b085-856f31124129
📒 Files selected for processing (2)
assets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_downloader.py
✅ Files skipped from review due to trivial changes (1)
- assets/docs/sources/tutorial/13_export_and_feature.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/jmcomic/jm_downloader.py (1)
313-314: ⚡ Quick winPreserve feature exception traceback in logs.
Current logging only interpolates
str(e), which loses stack context for failed features. Pass the exception object tojm_logso troubleshooting stays actionable.🔧 Proposed change
- except Exception as e: - jm_log('downloader.feature.exception', f'Feature执行失败: [{feature}], 来源: [{feature_from}], 异常: [{e}]') + except Exception as e: + jm_log( + 'downloader.feature.exception', + f'Feature执行失败: [{feature}], 来源: [{feature_from}], 异常: [{e}]', + 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 `@src/jmcomic/jm_downloader.py` around lines 313 - 314, The except block in jm_downloader.py currently logs only str(e) losing traceback; update the exception handling around the feature execution where jm_log is called (the except Exception as e block) to pass the exception object (e) as the exception/extra parameter to jm_log so the full traceback is preserved in logs; locate the jm_log call in the feature execution except block and change the call signature to include the exception object rather than interpolating its string representation.
🤖 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 `@src/jmcomic/jm_downloader.py`:
- Around line 313-314: The except block in jm_downloader.py currently logs only
str(e) losing traceback; update the exception handling around the feature
execution where jm_log is called (the except Exception as e block) to pass the
exception object (e) as the exception/extra parameter to jm_log so the full
traceback is preserved in logs; locate the jm_log call in the feature execution
except block and change the call signature to include the exception object
rather than interpolating its string representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66760b3a-ec63-4883-b907-2ff6710d758f
📒 Files selected for processing (7)
assets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_exception.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_toolkit.py
✅ Files skipped from review due to trivial changes (3)
- src/jmcomic/jm_client_impl.py
- src/jmcomic/jm_toolkit.py
- assets/docs/sources/tutorial/13_export_and_feature.md
hect0x7
changed the title
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_jmcomic/test_jm_feature.py (1)
153-153: 💤 Low valuePrefix unused
dlervariables with_to silence RUF059.♻️ Proposed fix
- album, dler = jmcomic.download_album(album_id, self.option, extra=combo) + album, _dler = jmcomic.download_album(album_id, self.option, extra=combo)- photo, dler = jmcomic.download_photo(photo_id, self.option, extra=f_pdf) + photo, _dler = jmcomic.download_photo(photo_id, self.option, extra=f_pdf)Also applies to: 177-177
🤖 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_feature.py` at line 153, The test assigns an unused variable named dler from jmcomic.download_album which triggers RUF059; rename the unused variable to _dler (or prefix it with an underscore) in the calls to jmcomic.download_album so that the result assignment becomes album, _dler (apply the same change for the other occurrence around the second call referenced) to silence the unused-variable lint warning while leaving jmcomic.download_album usage intact.src/jmcomic/jm_feature.py (1)
119-135: 💤 Low value
feature_fromparameter is accepted but never consulted inside_adapt_plugin_kwargs.The adaptation logic branches entirely on
when. SincePluginFeature.should_invokealready ensuresdownload_album → after_albumanddownload_photo → after_photoare the only reachable combinations,whenalone is sufficient here. The unused parameter is harmless but can mislead future contributors into thinking it influences the output.Consider either removing
feature_fromfrom the signature (if no future use is planned) or adding a brief comment explaining whywhenis sufficient.🤖 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 119 - 135, The _adapt_plugin_kwargs method declares a feature_from parameter but never uses it, which is misleading; either remove feature_from from the _adapt_plugin_kwargs signature (and update all callers) or keep it and add a concise comment in _adapt_plugin_kwargs explaining that PluginFeature.should_invoke guarantees the mapping (download_album→after_album, download_photo→after_photo) so only when is needed; ensure references to JmOption, plugin_key and kwargs remain unchanged and that behavior for filename_rule and dir mapping (zip_dir/pdf_dir/img_dir) is preserved.
🤖 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_option.py`:
- Around line 581-595: The OO wrappers JmOption.download_album and
JmOption.download_photo currently call the module-level
download_album/download_photo but discard their return values; update these
methods to return the underlying call's result so callers receive the (album,
dler) tuple. Specifically, in JmOption.download_album and
JmOption.download_photo, change the call pattern to return
download_album(album_id, self, *args, **kwargs) and return
download_photo(photo_id, self, *args, **kwargs) respectively so the returned
(album, dler) is propagated to callers.
---
Nitpick comments:
In `@src/jmcomic/jm_feature.py`:
- Around line 119-135: The _adapt_plugin_kwargs method declares a feature_from
parameter but never uses it, which is misleading; either remove feature_from
from the _adapt_plugin_kwargs signature (and update all callers) or keep it and
add a concise comment in _adapt_plugin_kwargs explaining that
PluginFeature.should_invoke guarantees the mapping (download_album→after_album,
download_photo→after_photo) so only when is needed; ensure references to
JmOption, plugin_key and kwargs remain unchanged and that behavior for
filename_rule and dir mapping (zip_dir/pdf_dir/img_dir) is preserved.
In `@tests/test_jmcomic/test_jm_feature.py`:
- Line 153: The test assigns an unused variable named dler from
jmcomic.download_album which triggers RUF059; rename the unused variable to
_dler (or prefix it with an underscore) in the calls to jmcomic.download_album
so that the result assignment becomes album, _dler (apply the same change for
the other occurrence around the second call referenced) to silence the
unused-variable lint warning while leaving jmcomic.download_album usage intact.
🪄 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: aba0b037-408b-44b1-b7dd-e7a0b094e777
📒 Files selected for processing (9)
assets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.pytests/test_jmcomic/test_jm_feature.py
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/docs/sources/option_file_syntax.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/jmcomic/jm_option.py (1)
581-595:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn value is still dropped in
JmOption.download_album/download_photo.Callers cannot receive the
(detail, downloader)tuple from API helpers, which makes Feature workflows harder to compose programmatically.🔧 Proposed fix
def download_album(self, album_id, *args, **kwargs, ): from .api import download_album - download_album(album_id, self, *args, **kwargs) + return download_album(album_id, self, *args, **kwargs) @@ def download_photo(self, photo_id, *args, **kwargs, ): from .api import download_photo - download_photo(photo_id, self, *args, **kwargs) + return download_photo(photo_id, self, *args, **kwargs)🤖 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_option.py` around lines 581 - 595, The methods JmOption.download_album and JmOption.download_photo are discarding the return value from the API helpers; change both methods to return the result of calling the imported functions (i.e., return download_album(album_id, self, *args, **kwargs) and return download_photo(photo_id, self, *args, **kwargs)) so callers receive the (detail, downloader) tuple from the API helpers.
🤖 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_option.py`:
- Around line 300-305: The current branch handling log='pretty' calls
enable_pretty_log() but does not re-enable the global logging gate set by
disable_jm_log(), so jm_log remains muted; update the jm_option.py branch that
handles log (around the block with log = dic.pop('log', True)) to both configure
pretty logging and restore the global flag—either call enable_jm_log() if that
function exists or set FLAG_ENABLE_JM_LOG = True after enable_pretty_log() (and
ensure FLAG_ENABLE_JM_LOG is imported/accessible) so pretty logging actually
produces output.
---
Duplicate comments:
In `@src/jmcomic/jm_option.py`:
- Around line 581-595: The methods JmOption.download_album and
JmOption.download_photo are discarding the return value from the API helpers;
change both methods to return the result of calling the imported functions
(i.e., return download_album(album_id, self, *args, **kwargs) and return
download_photo(photo_id, self, *args, **kwargs)) so callers receive the (detail,
downloader) tuple from the API helpers.
🪄 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: d5792d6d-0ddb-4f17-ab06-49dbbfac8660
📒 Files selected for processing (10)
assets/docs/sources/index.mdassets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.pytests/test_jmcomic/test_jm_feature.py
✅ Files skipped from review due to trivial changes (3)
- assets/docs/sources/index.md
- assets/docs/sources/option_file_syntax.md
- assets/docs/sources/tutorial/13_export_and_feature.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/jmcomic/jm_entity.py
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Owner
Author
|
@coderabbitai pause |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/jmcomic/jm_option.py (2)
581-595:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOO wrappers still drop the API return value.
api.download_album/api.download_photoreturn(album, dler)(relied on by the new tests, e.g.tests/test_jmcomic/test_jm_feature.pylines 153 and 177). The wrappers below silently discard that, so callers usingoption.download_album(id, extra=Feature.export_pdf)getNone— making the OO surface inconsistent with the module-level API and unable to inspect the downloader/album result.🔧 Proposed fix
def download_album(self, album_id, *args, **kwargs, ): from .api import download_album - download_album(album_id, self, *args, **kwargs) + return download_album(album_id, self, *args, **kwargs) def download_photo(self, photo_id, *args, **kwargs, ): from .api import download_photo - download_photo(photo_id, self, *args, **kwargs) + return download_photo(photo_id, self, *args, **kwargs)🤖 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_option.py` around lines 581 - 595, The OO wrapper methods download_album and download_photo currently call the module functions but discard their return values; update jm_option.py so both methods return the result of api.download_album(album_id, self, *args, **kwargs) and api.download_photo(photo_id, self, *args, **kwargs) respectively (preserving the (album, dler) tuple) so callers of Option.download_album and Option.download_photo receive the same return value as the module-level API.
300-305:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
log='pretty'still doesn't lift the global log gate.If anything earlier in the process called
disable_jm_log()(which setsFLAG_ENABLE_JM_LOG = False), constructing an option withlog: prettyconfigures the formatter/handler butJmModuleConfig.jm_logwill still short-circuit and emit nothing. This is exactly the path the newassets/option/option_workflow_download.yml(log: pretty) takes, so it's worth handling here.🔧 Proposed fix
log = dic.pop('log', True) if log is False: disable_jm_log() elif log == 'pretty': + JmModuleConfig.FLAG_ENABLE_JM_LOG = True enable_pretty_log() + else: + JmModuleConfig.FLAG_ENABLE_JM_LOG = bool(log)🤖 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_option.py` around lines 300 - 305, The current branch handling log == 'pretty' configures pretty logging but does not re-enable the global JM logging gate if it was previously disabled, so update the jm_option.py logic to both enable pretty formatting and lift the global flag: when log == 'pretty' call the routine that enables global JM logging (e.g., enable_jm_log() or set FLAG_ENABLE_JM_LOG = True) in addition to calling enable_pretty_log(); this ensures JmModuleConfig.jm_log will not short-circuit and pretty logs are actually emitted.
🤖 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.
Duplicate comments:
In `@src/jmcomic/jm_option.py`:
- Around line 581-595: The OO wrapper methods download_album and download_photo
currently call the module functions but discard their return values; update
jm_option.py so both methods return the result of api.download_album(album_id,
self, *args, **kwargs) and api.download_photo(photo_id, self, *args, **kwargs)
respectively (preserving the (album, dler) tuple) so callers of
Option.download_album and Option.download_photo receive the same return value as
the module-level API.
- Around line 300-305: The current branch handling log == 'pretty' configures
pretty logging but does not re-enable the global JM logging gate if it was
previously disabled, so update the jm_option.py logic to both enable pretty
formatting and lift the global flag: when log == 'pretty' call the routine that
enables global JM logging (e.g., enable_jm_log() or set FLAG_ENABLE_JM_LOG =
True) in addition to calling enable_pretty_log(); this ensures
JmModuleConfig.jm_log will not short-circuit and pretty logs are actually
emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 240886a9-fa26-4277-903b-bee253ae5f55
📒 Files selected for processing (10)
assets/docs/sources/index.mdassets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/13_export_and_feature.mdsrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.pytests/test_jmcomic/test_jm_feature.py
✅ Files skipped from review due to trivial changes (1)
- assets/docs/sources/index.md
🚧 Files skipped from review as they are similar to previous changes (2)
- assets/docs/sources/option_file_syntax.md
- src/jmcomic/jm_entity.py
✅ Actions performedReviews paused. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Around line 107-113: The filename example shown as “[章节标题].pdf” is
inconsistent with the download_photo default naming rule; update the example to
reflect the actual default pattern used by download_photo (i.e., include the
prefix pattern [JM{Pid}]{Ptitle} so the example becomes “[JM{Pid}]章节标题.pdf” or a
concrete example like “[JM456]章节标题.pdf”), and ensure the text referencing the
table entry and any tests match that same pattern (referencing the
download_photo default rule and the [JM{Pid}]{Ptitle} format).
In `@src/jmcomic/jm_downloader.py`:
- Around line 19-24: The new photo-failure append in the catch of
download_by_photo_detail is double-counting failures that originated in
download_by_image_detail; modify the catch block (the elif detail.is_photo()
branch in catch_exception around download_by_photo_detail) to only append to
self.download_failed_photo when the exception is a true photo-level failure
(i.e., not an exception raised/recorded by download_by_image_detail). Implement
this by detecting image-origin exceptions (for example by checking exception
type like ImageDownloadError or an origin attribute/set by
download_by_image_detail) and skipping the append when the exception indicates
an image-level failure; keep propagation (raise e) unchanged and ensure
raise_if_has_exception still reports correctly using download_failed_image vs
download_failed_photo.
In `@tests/test_jmcomic/test_jm_feature.py`:
- Line 153: The test calls jmcomic.download_album(...) return two values (album,
dler) but the second value is unused causing Ruff RUF059; update the test to
prefix the unused variable with an underscore (e.g., replace dler with _dler) in
both places where download_album is called (the call that assigns album, dler
and the later similar call) so the linter recognizes it as intentionally unused.
🪄 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: 4b8ae697-0772-47a5-8184-32d199d14340
📒 Files selected for processing (18)
README.mdassets/docs/mkdocs.ymlassets/docs/sources/index.mdassets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/13_export_and_feature.mdassets/option/option_workflow_download.ymlsrc/jmcomic/__init__.pysrc/jmcomic/api.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_exception.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_feature.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.
本次 PR 引入了全新的 Feature 导出系统，并针对导出插件、下载逻辑以及项目基础配置进行了全面优化和修复。
🚀 主要变更
Feature.export_xxx链式调用，大幅降低使用门槛。base_dir。alias_cn语义化标签（自动识别“本子/章节”）。[实体信息] → [文件路径]。log_enable的逻辑反转 Bug。APP_VERSION至2.0.21并刷新 API 域名列表。格式规范参考：
.github/release.pySummary by CodeRabbit
New Features
Improvements
Migration
Documentation
Tests
Version: 2.6.19