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 synchronous, asynchronous, threaded, and plugin execution paths. Public download callbacks are removed, exception options become keyword-only, logs include task identifiers, the CLI gains a deprecation shim, and documentation and tests are updated. ChangesTask Context and Download APIs
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: formatted 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
📒 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
📒 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
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
callbackparameters from single-album/single-photo APIs;check_exceptionis now keyword-only.