Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds task-local context propagation across download, executor, plugin, and logging paths. It adds album-comment pagination and parsing, removes public download callbacks, updates CLI compatibility, and revises dependencies, documentation, and tests. ChangesTask Context and Download APIs
Album Comment Pagination
Compatibility and Documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DownloadAPI
participant Executor
participant Logger
Caller->>DownloadAPI: start download
DownloadAPI->>DownloadAPI: set jm_task_context
DownloadAPI->>Executor: run bound task
Executor-->>Logger: emit contextual record
Logger-->>Caller: format task-aware log
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Clarify callback and batch exception contracts, add task-scoped logging across sync and async workers, and update the mobile API version.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/11_log_custom.md`:
- Line 117: Update the formatter assignment in the custom logging example to
avoid indexing jm_logger.handlers when no handlers are configured. Reuse the
first handler’s formatter when available, and otherwise apply a valid formatter
fallback before registering TaskLogHandler.
In `@assets/docs/sources/tutorial/4_module_custom.md`:
- Around line 20-22: Update the after_album override in the tutorial example to
delegate to JmDownloader.after_album by calling super().after_album(album),
while retaining the existing custom print behavior and choosing an appropriate
call order.
In `@README.md`:
- Line 241: Update the README bullet describing download event callbacks to
state that users must customize the behavior through a custom Downloader, such
as by subclassing or configuring it, rather than implying a public callback
parameter.
In `@src/jmcomic/__init__.py`:
- Line 7: Replace the star import from jm_task_context in the package
initializer with an explicit import of JM_TASK_CONTEXT, jm_task_context,
get_jm_task_context, and bind_jm_task_context, preserving the existing
package-level API.
In `@src/jmcomic/jm_task_context.py`:
- Around line 41-45: Update bind_jm_task_context to validate func with
callable(func) before async detection or wrapping, rejecting non-callable
targets immediately. Retain the existing __call__ inspection only for
identifying asynchronous callable objects, and preserve the current TypeError
behavior for unsupported async callables.
🪄 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 Plus
Run ID: 62f4de3f-407c-4dcd-b46a-c5266f1923e5
📥 Commits
Reviewing files that changed from the base of the PR and between 2d03218 and f2cd00f525aeac890b1bac9b50bd1386d8230574.
📒 Files selected for processing (18)
README.mdassets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/11_log_custom.mdassets/docs/sources/tutorial/13_export_and_feature.mdassets/docs/sources/tutorial/4_module_custom.mdsrc/jmcomic/__init__.pysrc/jmcomic/api.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_task_context.pytests/test_jmcomic/test_jm_api.pytests/test_jmcomic/test_jm_async_api.pytests/test_jmcomic/test_jm_task_context.py
💤 Files with no reviewable changes (1)
- tests/test_jmcomic/test_jm_async_api.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_config.py`:
- Line 612: Update the docstring for the ANSI-colored log formatter to replace
the fullwidth comma with an ASCII comma, preserving the existing wording and
behavior so Ruff’s RUF002 check passes.
🪄 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 Plus
Run ID: e859f80d-a09f-4951-b6c7-450765bd029c
📥 Commits
Reviewing files that changed from the base of the PR and between f2cd00f525aeac890b1bac9b50bd1386d8230574 and e39c191.
📒 Files selected for processing (18)
README.mdassets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/0_common_usage.mdassets/docs/sources/tutorial/11_log_custom.mdassets/docs/sources/tutorial/13_export_and_feature.mdassets/docs/sources/tutorial/4_module_custom.mdsrc/jmcomic/__init__.pysrc/jmcomic/api.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_downloader.pysrc/jmcomic/jm_feature.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_task_context.pytests/test_jmcomic/test_jm_api.pytests/test_jmcomic/test_jm_async_api.pytests/test_jmcomic/test_jm_task_context.py
💤 Files with no reviewable changes (1)
- tests/test_jmcomic/test_jm_async_api.py
🚧 Files skipped from review as they are similar to previous changes (11)
- README.md
- src/jmcomic/jm_plugin.py
- src/jmcomic/jm_feature.py
- src/jmcomic/jm_client_impl.py
- src/jmcomic/jm_downloader.py
- assets/docs/sources/option_file_syntax.md
- assets/docs/sources/tutorial/13_export_and_feature.md
- assets/docs/sources/tutorial/0_common_usage.md
- assets/docs/sources/tutorial/4_module_custom.md
- assets/docs/sources/tutorial/11_log_custom.md
- src/jmcomic/jm_async_downloader.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_config.py`:
- Line 23: Update the comment near the task_context logging logic in
jm_config.py by replacing both fullwidth commas with ASCII commas, resolving the
remaining RUF003 lint violation without changing the code behavior.
🪄 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 Plus
Run ID: ac8cb86d-a8a0-47ed-ac2b-4a2924e8179b
📒 Files selected for processing (6)
assets/docs/sources/api/command-line.mdsrc/jmcomic/cl.pysrc/jmcomic/jm_config.pytests/test_jmcomic/test_jm_cli.pytests/test_jmcomic/test_jm_task_context.pyusage/workflow_download.py
💤 Files with no reviewable changes (1)
- src/jmcomic/cl.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jmcomic/cl.py`:
- Line 3: Resolve the F403 lint error on the compatibility re-export in cl.py by
either replacing the wildcard import with an explicit __all__-driven export or
adding a narrowly scoped # noqa: F403 when preserving wildcard behavior is
required; keep the shim’s public API unchanged.
In `@tests/test_jmcomic/test_jm_cli.py`:
- Around line 13-17: Update test_cl_deprecated to remove the cached jmcomic.cl
entry from sys.modules before entering assertWarnsRegex, then perform the
deprecated import and preserve the existing identity assertion. Ensure the test
observes the module-level deprecation warning regardless of prior import order.
🪄 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 Plus
Run ID: bb4df826-9b7f-4a5d-9a6d-519d4ea757e6
📒 Files selected for processing (3)
assets/docs/sources/tutorial/11_log_custom.mdsrc/jmcomic/cl.pytests/test_jmcomic/test_jm_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/docs/sources/tutorial/11_log_custom.md
| @@ -1,9 +1,11 @@ | |||
| import warnings | |||
|
|
|||
| from .cli import * | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the intentional wildcard-import lint error.
Ruff reports F403 on this compatibility re-export. Keep the shim’s public surface explicit with an __all__-driven export, or add a narrowly scoped # noqa: F403 if the wildcard behavior is required.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 3-3: from .cli import * used; unable to detect undefined names
(F403)
🤖 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/cl.py` at line 3, Resolve the F403 lint error on the
compatibility re-export in cl.py by either replacing the wildcard import with an
explicit __all__-driven export or adding a narrowly scoped # noqa: F403 when
preserving wildcard behavior is required; keep the shim’s public API unchanged.
Source: Linters/SAST tools
Comment on lines
+13
to
+17
| def test_cl_deprecated(self): | ||
| with self.assertWarnsRegex(DeprecationWarning, r'removed in version 2\.7\.4'): | ||
| from jmcomic.cl import JmcomicUI as DeprecatedJmcomicUI | ||
|
|
||
| self.assertIs(DeprecatedJmcomicUI, JmcomicUI) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(test_jm_cli\.py|cl\.py)$|src/jmcomic/cl\.py' || true
echo "== target test =="
cat -n tests/test_jmcomic/test_jm_cli.py | sed -n '1,80p'
echo "== cl.py warning code =="
cat -n src/jmcomic/cl.py | sed -n '1,120p'
echo "== imports of jmcomic.cl =="
rg -n "from jmcomic\.cl import|import jmcomic\.cl|jmcomic\.cl" tests src || true
echo "== Python module import behavior probe =="
python3 - <<'PY'
import warnings
import tempfile
import pathlib
p = pathlib.Path(tempfile.mkdtemp(), "mymod.py")
p.write_text("import warnings\nwarnings.warn('cached?', DeprecationWarning, stacklevel=1)\nvalue = 1\n")
warnings.filterwarnings("always", category=DeprecationWarning)
from importlib import import_module, reload
import sys
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always", DeprecationWarning)
m = import_module(p.stem)
print("first_import_values:", m.value, "warnings:", len(w), [str(x.message) for x in w if x.category == DeprecationWarning])
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always", DeprecationWarning)
loaded = import_module(p.stem)
print("second_import_same_values:", m is loaded, loaded.value, "warnings:", len(w), [str(x.message) for x in w if x.category == DeprecationWarning])
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always", DeprecationWarning)
reloaded = reload(m)
print("reload_same_values:", reloaded is m, reloaded.value, "warnings:", len(w), [str(x.message) for x in w if x.category == DeprecationWarning])
del sys.modules[m.__name__]
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always", DeprecationWarning)
fresh = import_module(p.stem)
print("fresh_after_reload_import_values:", fresh.value, "warnings:", len(w), [str(x.message) for x in w if x.component.category == DeprecationWarning for x in w] if False else len([x.message for x in w if x.category == DeprecationWarning]))
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 4833
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test setup/base =="
for f in $(git ls-files | rg '(^|/)test_jmcomic/test_configurable\.py$|(^|/)(test_.*\.py|.*\.py)$' | rg 'test_jmcomic'); do
echo "--- $f"
wc -l "$f"
done
cat -n tests/test_jmcomic/test_configurable.py | sed -n '1,220p' || true
echo "== jmcomic package imports for cl imports =="
rg -n "from jmcomic\.cl import|import jmcomic\.cl|jmcomic\.cl" tests/jmcomic tests src || true
echo "== Python module caching probe (no installed package) =="
python3 - <<'PY'
import warnings
import tempfile
import pathlib
import sys
d = pathlib.Path(tempfile.mkdtemp())
(d / "pkg").mkdir()
(d / "pkg" / "__init__.py").write_text("")
(d / "pkg" / "pkgmod.py").write_text("""
import warnings
warnings.warn("cached?", DeprecationWarning, stacklevel=3)
value = 1
""")
sys.path.insert(0, str(d))
del sys.modules["pkg"]
if "pkg.pkgmod" in sys.modules:
del sys.modules["pkg.pkgmod"]
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
import pkg.pkgmod # first import
print("first_import_first_loaded", pkg.pkgmod is sys.modules.get("pkg.pkgmod"))
print("first_import_warning_count", len(w), [str(m.message) for m in w if m.category is DeprecationWarning])
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
from pkg.pkgmod import pkgmod # cached import
print("cached_after_cached_import_warning_count", len(w), [str(m.message) for m in w if m.category is DeprecationWarning])
from importlib import reload
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
reload(pkg.pkgmod)
print("reload_import_warning_count", len(w), [str(m.message) for m in w if m.category is DeprecationWarning])
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 1902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test init =="
cat -n tests/test_jmcomic/__init__.py | sed -n '1,220p'
echo "== current test diff/stat if available =="
git diff -- tests/test_jmcomic/test_jm_cli.py src/jmcomic/cl.py tests/test_jmcomic/test_jm_cli.py | sed -n '1,220p'
echo "== Python import cache behavior with real module shape (standalone temp package) =="
python3 - <<'PY'
import warnings
import tempfile
import pathlib
import sys
import os
d = pathlib.Path(tempfile.mkdtemp())
(d / "pkg").mkdir()
(d / "pkg" / "__init__.py").write_text("from .pkgmod import *\n")
(d / "pkg" / "pkgmod.py").write_text("""
import warnings
warnings.warn("pkgmod cached warning", DeprecationWarning, stacklevel=2)
value = 1
""")
sys.path.insert(0, str(d))
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
import pkg
print("first_import_warnings", len(w), [str(x.message) for x in w if x.category is DeprecationWarning])
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
from pkg import pkgmod
print("cached_import_warnings", len(w), [str(x.message) for x in w if x.category is DeprecationWarning])
from importlib import reload
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
reload(pkg.pkgmod)
print("reload_after_cached_warnings", len(w), [str(x.message) for x in w if x.category is DeprecationWarning])
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 7917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package exports =="
sed -n '1,220p' src/jmcomic/__init__.py
echo "---"
fd -e py . src/jmcomic | sort | while read -r f; do
echo "--- $f"
rg -n "from \.cli import|from \. import|import .*cl|cli\.|JmcomicUI|JmViewUI|warnings\.warn" "$f" || true
done
echo "== jmcomic subpackage cl import from cli =="
rg -n "from \.cli import|warnings\.warn" src/jmcomic || trueRepository: hect0x7/JMComic-Crawler-Python
Length of output: 2617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api.py get_env import section =="
sed -n '120,170p' src/jmcomic/api.py
echo "== all direct references to jmcomic.cli in src/tests =="
rg -n "jmcomic\.cli|\.cli import|from \.cl import|import\s+cl|cl\.py" src tests || true
echo "== Python behavior probe using the exact jmcomic source relationship =="
tmp="$(mktemp -d)"
mkdir -p "$tmp/jmcomic"
mkdir -p "$tmp/jmcomic/tests"
cat > "$tmp/jmcomic/__init__.py" <<'PY'
from .jm_plugin import *
from .api import *
PY
cat > "$tmp/jmcomic/jm_plugin.py" <<'PY'
pass
PY
cat > "$tmp/jmcomic/api.py" <<'PY'
def get_env():
from .cli import JmcomicUI
return JmcomicUI
PY
cat > "$tmp/jmcomic/cli.py" <<'PY'
class JmcomicUI:
pass
PY
cat > "$tmp/jmcomic/cl.py" <<'PY'
import warnings
from .cli import *
warnings.warn("removed in version 2.7.4", DeprecationWarning, stacklevel=2)
PY
cat > "$tmp/jmcomic/tests/__init__.py" <<'PY'
import jmcomic
from jmcomic import *
PY
cat > "$tmp/tests.py" <<'PY'
from unittest import TestCase
import warnings
class TestCli(TestCase):
def test_cl_deprecated_after_setup_imports_cli(self):
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always", DeprecationWarning)
from jmcomic.cl import JmcomicUI as DeprecatedJmcomicUI
print("warnings", len(w), [str(x.message) for x in w if x.category is DeprecationWarning])
print("assertIs", DeprecatedJmcomicUI)
if __name__ == "__main__":
import sys
import unittest
import jmcomic.tests
unittest.main(argv=[sys.argv[0], "-v"])
PY
python3 - <<'PY'
import sys, warnings
warnings.filterwarnings("always", category=DeprecationWarning)
sys.path.insert(0, '/tmp')
class TC: pass
from jmcomic.tests import import_jmcomic_tests
print("setup_imported_jmcomic_tests", import_jmcomic_tests)
print("jmcomic_module_in_sys_modules", "jmcomic.cl" in sys.modules)
warnings.simplefilter("always", DeprecationWarning)
from jmcomic.cl import JmcomicUI as DeprecatedJmcomicUI
print("cached_deprecated_import_warnings", len([x for x in dir(TC) if False]))
PYRepository: hect0x7/JMComic-Crawler-Python
Length of output: 2241
Make the deprecation test independent of module-import order.
jmcomic.cl emits its DeprecationWarning during module execution, but Python caches that module after the package test setup imports jmcomic; a later from jmcomic.cl import ... inside the assertWarnsRegex context will reuse the cached module and can fail to see the warning. Reloading is not enough in the normal test setup; isolate the import by removing jmcomic.cl from sys.modules before asserting.
🤖 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_cli.py` around lines 13 - 17, Update
test_cl_deprecated to remove the cached jmcomic.cl entry from sys.modules before
entering assertWarnsRegex, then perform the deprecated import and preserve the
existing identity assertion. Ensure the test observes the module-level
deprecation warning regardless of prior import order.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_jmcomic/test_jm_task_context.py (1)
133-168: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert rendered task-context fields, not only color prefixes.
This test supplies
task_id,download_type, andjm_id, but only checksstartswith(...). A formatter that drops task-context data entirely would still pass. Assert that the formatted output contains the required identifiers to cover the logging contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_task_context.py` around lines 133 - 168, Update test_pretty_formatter_uses_topic_colors to retain its existing color-prefix assertions and also verify that formatted output includes the task_context identifiers task-A, album, and 1. Assert these values on the rendered output so the test covers both color selection and task-context field rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_jmcomic/test_jm_task_context.py`:
- Around line 133-168: Update test_pretty_formatter_uses_topic_colors to retain
its existing color-prefix assertions and also verify that formatted output
includes the task_context identifiers task-A, album, and 1. Assert these values
on the rendered output so the test covers both color selection and task-context
field rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b306214b-cdc1-4ec4-a64f-f64113218d90
📒 Files selected for processing (1)
tests/test_jmcomic/test_jm_task_context.py
hect0x7
changed the title
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/jmcomic/jm_toolkit.py (2)
801-802: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or use the unused
pageparameter.
parse_api_to_album_comment_pageandparse_html_to_album_comment_pageboth acceptpagebut never read it. The callers insrc/jmcomic/jm_client_impl.pyandsrc/jmcomic/jm_async_client.pypass the value. Either store the page number on the returnedJmAlbumCommentPageor drop the parameter.Also applies to: 834-835
🤖 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_toolkit.py` around lines 801 - 802, Update parse_api_to_album_comment_page and parse_html_to_album_comment_page to remove the unused page parameter, then adjust their callers in JmClientImpl and JmAsyncClient so they no longer pass it; preserve the existing JmAlbumCommentPage construction and behavior.
537-538: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMove the
urllib.parseimport out ofhandle_starttag.
handle_starttagruns once per HTML tag. The import statement executes on every call. Python caches modules, but the lookup still adds overhead in the parse loop. Move the import to module scope or to the class body.🤖 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_toolkit.py` around lines 537 - 538, Move the urllib.parse imports used by handle_starttag to module scope or the containing class body, and remove the per-call import from handle_starttag while preserving its existing unquote and urlparse behavior.tests/test_jmcomic/test_jm_client.py (1)
235-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the last domain error for the failure message.
The loop discards every exception. If all domains fail, the assertion reports only "所有网页域名均无法获取本子评论". The cause is lost, and the diagnosis of a CI failure becomes hard. Keep the last exception and include it in the message. This also resolves the Ruff
S112andBLE001hints.♻️ Proposed change
html_page = None + last_error = None for domain in html_client.get_html_domain_all(): html_client.set_domain_list([domain]) try: html_page = html_client.album_pagination(album_id, page=1) break - except Exception: + except Exception as e: + last_error = e continue - self.assertIsNotNone(html_page, '所有网页域名均无法获取本子评论') + self.assertIsNotNone(html_page, f'所有网页域名均无法获取本子评论, 最后一个错误: {last_error}')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jmcomic/test_jm_client.py` around lines 235 - 244, Update the domain-retry loop around album_pagination to retain the most recent exception instead of discarding it, while continuing to try remaining domains. Include that exception in the final assertIsNotNone failure message so all-domain failures report the underlying cause and satisfy Ruff S112/BLE001.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jmcomic/jm_client_interface.py`:
- Around line 1052-1053: Update the async pagination loop around
album_pagination_gen so page_count being None does not immediately break after
the first page; continue fetching pages until a response contains no comments,
while preserving the existing page_count boundary when it is available.
Optionally log the missing or malformed total using the surrounding pagination
logger.
---
Nitpick comments:
In `@src/jmcomic/jm_toolkit.py`:
- Around line 801-802: Update parse_api_to_album_comment_page and
parse_html_to_album_comment_page to remove the unused page parameter, then
adjust their callers in JmClientImpl and JmAsyncClient so they no longer pass
it; preserve the existing JmAlbumCommentPage construction and behavior.
- Around line 537-538: Move the urllib.parse imports used by handle_starttag to
module scope or the containing class body, and remove the per-call import from
handle_starttag while preserving its existing unquote and urlparse behavior.
In `@tests/test_jmcomic/test_jm_client.py`:
- Around line 235-244: Update the domain-retry loop around album_pagination to
retain the most recent exception instead of discarding it, while continuing to
try remaining domains. Include that exception in the final assertIsNotNone
failure message so all-domain failures report the underlying cause and satisfy
Ruff S112/BLE001.
🪄 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 Plus
Run ID: da218ec2-4d33-47ad-95bb-9ed6b468549b
📒 Files selected for processing (16)
assets/docs/sources/api/entity.mdassets/docs/sources/index.mdassets/docs/sources/option_file_syntax.mdassets/docs/sources/tutorial/0_common_usage.mdrequirements-dev.txtsrc/jmcomic/jm_async_client.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_client_interface.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_entity.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.pytests/test_jmcomic/test_jm_api.pytests/test_jmcomic/test_jm_async_client.pytests/test_jmcomic/test_jm_client.pytests/test_jmcomic/test_jm_task_context.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_jmcomic/test_jm_api.py
- tests/test_jmcomic/test_jm_task_context.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.
Clarify callback and batch exception contracts, add task-scoped logging across sync and async workers, and update the mobile API version.
Summary by CodeRabbit
check_exceptionis now keyword-only.