Conversation
hect0x7
changed the title
📝 WalkthroughWalkthroughBumped package and app versions; replaced strict boolean comparisons with truthiness; added album fallback when Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jmcomic/jm_plugin.py (1)
38-45:⚠️ Potential issue | 🟠 MajorPlugin logging is inverted here.
self.log_enabledefaults toTrue, but this branch returns before emitting anything. Combined withJmOption.invoke_plugin()settinglog_enable = Falseforpinfo['log']=False, plugin logs are now suppressed by default and only show up when logging is disabled in config.Suggested fix
def log(self, msg, topic=None): - if self.log_enable: + if not self.log_enable: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_plugin.py` around lines 38 - 45, The log method is inverted: it returns early when self.log_enable is truthy, causing logs to be suppressed by default; change the conditional in log (method name: log, attribute: self.log_enable) so that it only returns when logging is disabled (i.e., if not self.log_enable return), then call jm_log with the same topic construction (using self.plugin_key and optional topic) and msg; also ensure any places that set pinfo['log'] (e.g., JmOption.invoke_plugin) keep the intended semantics (True = enable, False = disable).src/jmcomic/jm_option.py (1)
525-546:⚠️ Potential issue | 🟠 Major
safe=Falseno longer forces fail-fast plugin execution.With the current condition, passing
safe=Falsestill swallows exceptions wheneverpinfo['safe']is omitted orTrue. IfNoneis the new "defer to plugin config" sentinel, the method-level override should win wheneversafeis explicitly set.Suggested fix
- def call_all_plugin(self, group: str, safe=None, **extra): + def call_all_plugin(self, group: str, safe: Optional[bool] = None, **extra): plugin_list: List[dict] = self.plugins.get(group, []) if plugin_list is None or len(plugin_list) == 0: return @@ try: self.invoke_plugin(pclass, kwargs, extra, pinfo) except BaseException as e: - if safe is True or pinfo.get('safe', True): + swallow = pinfo.get('safe', True) if safe is None else safe + if swallow: jm_log('plugin.exception', e) else: raise e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_option.py` around lines 525 - 546, In call_all_plugin, the exception-swallowing logic ignores an explicit safe=False because it falls back to pinfo.get('safe', True); change the decision to honor an explicit method-level safe when provided: compute an effective_safe = safe if safe is not None else pinfo.get('safe', True) (or equivalent conditional: if safe is True or (safe is None and pinfo.get('safe', True'))) and use effective_safe when deciding whether to log and continue or re-raise after invoking invoke_plugin; reference pinfo, safe, pinfo.get('safe', True), and invoke_plugin to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/jmcomic/jm_option.py`:
- Around line 300-302: The code treats any falsy `log` value as "off" which
turns off logging when `log` is None (the sentinel for "inherit default");
change the pop to preserve the None sentinel and only disable logging on an
explicit False: use dic.pop('log', None) and call disable_jm_log() only when the
popped `log` is exactly False (e.g., if log is False: disable_jm_log()). This
keeps the JmModuleConfig.DEFAULT_OPTION_DICT['log'] = None behavior and allows
FLAG_ENABLE_JM_LOG to control inheritance.
In `@src/jmcomic/jm_plugin.py`:
- Around line 71-72: The guard in execute_deletion is inverted: currently it
returns when self.delete_original_file is True which means deletion runs by
default; change the condition to "if not self.delete_original_file: return" so
deletion is skipped unless the user opted in; update the check in the helper
method (execute_deletion) referenced by Img2pdfPlugin, LongImgPlugin, and
ZipPlugin to use the corrected boolean logic and ensure the default False for
delete_original_file prevents removal.
In `@tests/test_jmcomic/test_jm_plugin.py`:
- Around line 23-36: The test currently calls download_photo(...) which turns a
local path-resolution regression into a live API test and can false-pass when
img2pdf or Pillow aren’t installed; change the test to be self-contained by
constructing local JmAlbumDetail and JmPhotoDetail fixtures (or monkeypatch the
plugins' imports/I/O) and invoke the plugins' path-generation logic directly
(e.g., call Img2pdfPlugin.invoke and LongImgPlugin.invoke or their internal
path-generation helpers) instead of calling download_photo; ensure
option.plugins['before_photo'] still contains the same entries and assert the
generated path(s) and dir_rule handling, keeping DoNotDownloadImage out of the
execution path so no real network or optional dependency is required.
---
Outside diff comments:
In `@src/jmcomic/jm_option.py`:
- Around line 525-546: In call_all_plugin, the exception-swallowing logic
ignores an explicit safe=False because it falls back to pinfo.get('safe', True);
change the decision to honor an explicit method-level safe when provided:
compute an effective_safe = safe if safe is not None else pinfo.get('safe',
True) (or equivalent conditional: if safe is True or (safe is None and
pinfo.get('safe', True'))) and use effective_safe when deciding whether to log
and continue or re-raise after invoking invoke_plugin; reference pinfo, safe,
pinfo.get('safe', True), and invoke_plugin to locate the change.
In `@src/jmcomic/jm_plugin.py`:
- Around line 38-45: The log method is inverted: it returns early when
self.log_enable is truthy, causing logs to be suppressed by default; change the
conditional in log (method name: log, attribute: self.log_enable) so that it
only returns when logging is disabled (i.e., if not self.log_enable return),
then call jm_log with the same topic construction (using self.plugin_key and
optional topic) and msg; also ensure any places that set pinfo['log'] (e.g.,
JmOption.invoke_plugin) keep the intended semantics (True = enable, False =
disable).
🪄 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: 1c690c1d-c066-4b22-81ec-57b586301990
📒 Files selected for processing (5)
src/jmcomic/__init__.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.pytests/test_jmcomic/test_jm_plugin.py
✅ Files skipped from review due to trivial changes (1)
- src/jmcomic/init.py
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/test_jm_custom.py (1)
24-40:⚠️ Potential issue | 🟠 MajorRestore global config overrides in
finallyand reset to true default state.Cleanup at Line 39-40 won’t run if the assertion fails, so
JmModuleConfigcan leak mutated global state into later tests. Also, resetting toJmAlbumDetail/JmPhotoDetaildoes not restore the class-level default state (None).🔧 Proposed fix
@@ - JmModuleConfig.CLASS_ALBUM = MyAlbum - JmModuleConfig.CLASS_PHOTO = MyPhoto - - base_dir: str = workspace() - dir_rule = DirRule('Bd_Aaname_Ppname', base_dir) - # noinspection PyTypeChecker - save_dir = dir_rule.decide_image_save_dir( - MyAlbum('1', '0', '0', [], *['0'] * 10), - MyPhoto('2', *['0'] * 7) - ) - - self.assertEqual( - os.path.abspath(save_dir), - os.path.abspath(base_dir + dic[1] + '/' + dic[2]), - ) - JmModuleConfig.CLASS_ALBUM = JmAlbumDetail - JmModuleConfig.CLASS_PHOTO = JmPhotoDetail + prev_album_cls = JmModuleConfig.CLASS_ALBUM + prev_photo_cls = JmModuleConfig.CLASS_PHOTO + JmModuleConfig.CLASS_ALBUM = MyAlbum + JmModuleConfig.CLASS_PHOTO = MyPhoto + try: + base_dir: str = workspace() + dir_rule = DirRule('Bd_Aaname_Ppname', base_dir) + # noinspection PyTypeChecker + save_dir = dir_rule.decide_image_save_dir( + MyAlbum('1', '0', '0', [], *['0'] * 10), + MyPhoto('2', *['0'] * 7) + ) + + self.assertEqual( + os.path.abspath(save_dir), + os.path.abspath(base_dir + dic[1] + '/' + dic[2]), + ) + finally: + JmModuleConfig.CLASS_ALBUM = prev_album_cls + JmModuleConfig.CLASS_PHOTO = prev_photo_cls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_jmcomic/test_jm_custom.py` around lines 24 - 40, Capture the original JmModuleConfig.CLASS_ALBUM and CLASS_PHOTO before you override them, wrap the test mutation and assertions in a try block, and in a finally block restore those captured originals (or set them back to None) so the global config is always reset even if the assertion fails; update the mutation lines that set JmModuleConfig.CLASS_ALBUM = MyAlbum and JmModuleConfig.CLASS_PHOTO = MyPhoto to follow this pattern so cleanup is guaranteed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/test_jmcomic/test_jm_custom.py`:
- Around line 24-40: Capture the original JmModuleConfig.CLASS_ALBUM and
CLASS_PHOTO before you override them, wrap the test mutation and assertions in a
try block, and in a finally block restore those captured originals (or set them
back to None) so the global config is always reset even if the assertion fails;
update the mutation lines that set JmModuleConfig.CLASS_ALBUM = MyAlbum and
JmModuleConfig.CLASS_PHOTO = MyPhoto to follow this pattern so cleanup is
guaranteed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f78d8a6-5593-44f5-b256-afbbddd478a7
📒 Files selected for processing (1)
tests/test_jmcomic/test_jm_custom.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jmcomic/jm_plugin.py (1)
38-45:⚠️ Potential issue | 🔴 CriticalLogic is inverted: logging is disabled when
log_enableisTrue.The condition
if self.log_enable: returncauses the method to skip logging whenlog_enableis truthy. Sincelog_enabledefaults toTrue(line 20), logging will never occur. The condition should be inverted.🐛 Proposed fix
def log(self, msg, topic=None): - if self.log_enable: + if not self.log_enable: return jm_log(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_plugin.py` around lines 38 - 45, The log method's guard is inverted: it returns when self.log_enable is True so logging never happens; update the condition in jm_plugin.py inside def log(self, msg, topic=None) to return early when logging is disabled (e.g., use if not self.log_enable: return) and leave the jm_log call using self.plugin_key and optional topic unchanged so messages are emitted when log_enable is True.
🧹 Nitpick comments (1)
src/jmcomic/jm_plugin.py (1)
869-884: Minor: Log message references empty list instead of searched directories.At line 882,
img_pathsis logged but it's already an empty list at this point. The message would be more helpful if it loggedimg_dir_items(the directories that were searched) instead of the empty result.📝 Suggested improvement
if not img_paths: - self.log(f'所有文件夹都不存在图片，无法生成long_img：{img_paths}', 'error') + self.log(f'所有文件夹都不存在图片，无法生成long_img：{img_dir_items}', 'error') return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_plugin.py` around lines 869 - 884, In write_img_2_long_img, the error log prints img_paths (already empty) instead of the directories searched; update the process to log img_dir_items (the list of directories generated by option.decide_image_save_dir / album mapping) in the self.log call so the message shows which folders were searched when no images were found (keep the same message text but replace the referenced variable from img_paths to img_dir_items and leave the subsequent return as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/jmcomic/jm_plugin.py`:
- Around line 123-127: The fallback that sets album = photo.from_album can still
leave album as None if photo.from_album is unset (JmPhotoDetail), so update the
album handling in the method containing the lines setting album to
photo.from_album to defensively handle that case: after the current fallback
check, if album is still None log a warning (or raise a clear error) and
short-circuit before calling DirRule/placeholder parsing (mirroring the
defensive pattern used in apply_rule_to_filename in jm_option.py), ensuring any
downstream calls that expect a non-None album do not receive None.
---
Outside diff comments:
In `@src/jmcomic/jm_plugin.py`:
- Around line 38-45: The log method's guard is inverted: it returns when
self.log_enable is True so logging never happens; update the condition in
jm_plugin.py inside def log(self, msg, topic=None) to return early when logging
is disabled (e.g., use if not self.log_enable: return) and leave the jm_log call
using self.plugin_key and optional topic unchanged so messages are emitted when
log_enable is True.
---
Nitpick comments:
In `@src/jmcomic/jm_plugin.py`:
- Around line 869-884: In write_img_2_long_img, the error log prints img_paths
(already empty) instead of the directories searched; update the process to log
img_dir_items (the list of directories generated by option.decide_image_save_dir
/ album mapping) in the self.log call so the message shows which folders were
searched when no images were found (keep the same message text but replace the
referenced variable from img_paths to img_dir_items and leave the subsequent
return as-is).
🪄 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: 0faefae8-a715-4b8d-916f-34309e154496
📒 Files selected for processing (3)
requirements-dev.txtsrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.py
✅ Files skipped from review due to trivial changes (1)
- requirements-dev.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/jmcomic/jm_option.py
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
Bug Fixes
Refactor
Chores
Tests