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:
📝 WalkthroughWalkthroughRefactors logging to a module-level Python logger with topic-aware filtering and exception support, updates jm_log signature and related callers to accept an optional Exception, amends docs/examples for the new logging API, raises minimum Python to >=3.9 (recommends 3.12+), bumps version to 2.6.15, updates CI matrix, and adds a contributor guide. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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
🧹 Nitpick comments (3)
src/jmcomic/jm_config.py (3)
27-27:msgis untyped andeuses implicitOptional— two annotation gaps flagged by Ruff (RUF013).
default_jm_loggingacceptsmsgas eitherstrorBaseException(line 29 checksisinstance(msg, BaseException)), but the parameter has no annotation. Similarly,e: Exception = Noneis an implicitOptional[Exception].♻️ Proposed annotation fix
-def default_jm_logging(topic: str, msg, e: Exception = None): +def default_jm_logging(topic: str, msg: Union[str, BaseException], e: Optional[Exception] = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` at line 27, The function default_jm_logging has missing type annotations: annotate msg as Union[str, BaseException] and annotate e as Optional[Exception] to remove the implicit Optional warning; update the function signature for default_jm_logging to use these types and add the necessary typing imports (Union, Optional) at the top of the module so type checkers and Ruff (RUF013) are satisfied.
16-24:setup_default_jm_loggeradds a handler at library import time — deviates from Python library logging best practice.Python's logging HOWTO recommends that libraries add only a
NullHandlerand leave handler configuration to application code. Theif not jm_logger.handlers:guard is a reasonable mitigation for backward compatibility (as noted in the comment), but be aware this means importingjmcomicin an application that hasn't pre-configured the"jmcomic"logger will always attach aStreamHandlerand set the level toINFO. This could interfere with host applications that rely onpropagate=Trueand root-logger-level control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 16 - 24, setup_default_jm_logger currently attaches a StreamHandler and sets jm_logger level at import, which violates library logging best practices; change it to only add a logging.NullHandler (and do not call jm_logger.setLevel) when no handlers exist so the library does not configure application logging, and keep the import-time guard and jm_logger symbol but replace the StreamHandler/formatter/level actions with adding logging.NullHandler() to jm_logger to preserve backward compatibility without forcing console output.
408-410:jm_log'smsg: strannotation contradicts its documented shortcut usage; also implicitOptional(Ruff RUF013).The method is designed to accept an exception object as
msg(e.g.,jm_log('plugin.exception', e)), but the signature declaresmsg: str. This misleads callers and type checkers. Thee: Exception = Noneshould also beOptional[Exception] = None.♻️ Proposed annotation fix
- def jm_log(cls, topic: str, msg: str, e: Exception = None): + def jm_log(cls, topic: str, msg: Union[str, BaseException], e: Optional[Exception] = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 408 - 410, The jm_log signature mis-annotates parameters: change msg: str to accept non-string exceptions (e.g., msg: Any or msg: Union[str, Exception]) and annotate the exception parameter as e: Optional[Exception] = None (import Optional/Any or Union from typing); update the function signature on jm_log and add the necessary typing import so cls.EXECUTOR_LOG(topic, msg, e) remains unchanged and Ruff RUF013 is satisfied while callers like jm_log('plugin.exception', e) are type-correct.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CONTRIBUTING.mdassets/docs/sources/tutorial/11_log_custom.mdassets/docs/sources/tutorial/4_module_custom.mdsrc/jmcomic/__init__.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_exception.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.py
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 55: Replace the incorrect use of the particle "的" with the adverbial
particle "地" in the two phrases flagged by the reviewer: change "最新的推流" to
"最新地推流" and change "合理的命名" to "合理地命名" in CONTRIBUTING.md; locate the exact
phrases (quoted above) and update them so the verb-modifying phrases use "地".
In `@src/jmcomic/jm_config.py`:
- Line 21: The logging Formatter in jm_config.py uses '%(topic)s' causing
KeyError when jm_logger.* is called without extra; update the logging.Formatter
instantiation (the formatter variable) to supply a defaults mapping (e.g.
include a 'topic' default) so records missing 'topic' won't error — adjust the
call that creates formatter in jm_config.py (where logging.Formatter(...) is
used) to pass defaults={'topic': '<default value or empty string>'} compatible
with Python 3.8+.
In `@src/jmcomic/jm_plugin.py`:
- Around line 558-567: TopicFilter is defined inside invoke so each call creates
a distinct class and isinstance checks against jm_logger.filters fail to remove
prior instances; move TopicFilter out of invoke to module scope (or make it a
nested class on a stable object) so its identity is stable, then retain the
deduplication logic that filters jm_logger.filters by isinstance(f, TopicFilter)
and re-add a single TopicFilter instance; update references in log_topic_filter
/ JmOption after_init to use the module-level TopicFilter.
---
Nitpick comments:
In `@src/jmcomic/jm_config.py`:
- Line 27: The function default_jm_logging has missing type annotations:
annotate msg as Union[str, BaseException] and annotate e as Optional[Exception]
to remove the implicit Optional warning; update the function signature for
default_jm_logging to use these types and add the necessary typing imports
(Union, Optional) at the top of the module so type checkers and Ruff (RUF013)
are satisfied.
- Around line 16-24: setup_default_jm_logger currently attaches a StreamHandler
and sets jm_logger level at import, which violates library logging best
practices; change it to only add a logging.NullHandler (and do not call
jm_logger.setLevel) when no handlers exist so the library does not configure
application logging, and keep the import-time guard and jm_logger symbol but
replace the StreamHandler/formatter/level actions with adding
logging.NullHandler() to jm_logger to preserve backward compatibility without
forcing console output.
- Around line 408-410: The jm_log signature mis-annotates parameters: change
msg: str to accept non-string exceptions (e.g., msg: Any or msg: Union[str,
Exception]) and annotate the exception parameter as e: Optional[Exception] =
None (import Optional/Any or Union from typing); update the function signature
on jm_log and add the necessary typing import so cls.EXECUTOR_LOG(topic, msg, e)
remains unchanged and Ruff RUF013 is satisfied while callers like
jm_log('plugin.exception', e) are type-correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CONTRIBUTING.md (2)
55-55:⚠️ Potential issue | 🟡 MinorGrammar nit:
最新的推流→最新地推流(adverbial地, not attributive的).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 55, Replace the grammatically incorrect phrase "最新的推流" with the adverbial form "最新地推流" in the CONTRIBUTING.md sentence "💡 请前往你个人仓库的 **Actions** 面板，确保最新的推流全部亮起绿灯。" so it reads "💡 请前往你个人仓库的 **Actions** 面板，确保最新地推流全部亮起绿灯。"; update only that phrase to correct the adverbial usage.
86-86:⚠️ Potential issue | 🟡 MinorGrammar nit:
合理的命名→合理地命名(adverbial地, not attributive的).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 86, Replace the grammatically incorrect phrase "合理的命名" with the adverbial form "合理地命名" in the CONTRIBUTING.md sentence that currently reads "在为 `jmcomic` 编写或改造代码时，请尽量增加类型注解 (Type hints) 并遵守合理的命名。除此之外，为确保全机队的统一性，请尽量遵循以下架构规范："; update that exact string so it becomes "并遵守合理地命名" to correct the grammar.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CONTRIBUTING.mdREADME.mdassets/readme/README-en.mdassets/readme/README-jp.mdassets/readme/README-kr.mdpyproject.tomlsetup.py
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 38: The internal Markdown anchor for the heading "🛠️ 引擎改装规范 (Coding
Guidelines)" is broken because GitHub strips emoji and produces a different
anchor; fix it by adding an explicit HTML anchor immediately above that heading
(e.g., <a name="coding-guidelines"></a>) and then update the existing link that
currently points to the emoji-based anchor to use (`#coding-guidelines`) instead;
locate the heading text "🛠️ 引擎改装规范 (Coding Guidelines)" and the link text
referencing that heading and change them as described.
---
Duplicate comments:
In `@CONTRIBUTING.md`:
- Line 55: Replace the grammatically incorrect phrase "最新的推流" with the adverbial
form "最新地推流" in the CONTRIBUTING.md sentence "💡 请前往你个人仓库的 **Actions**
面板，确保最新的推流全部亮起绿灯。" so it reads "💡 请前往你个人仓库的 **Actions** 面板，确保最新地推流全部亮起绿灯。";
update only that phrase to correct the adverbial usage.
- Line 86: Replace the grammatically incorrect phrase "合理的命名" with the adverbial
form "合理地命名" in the CONTRIBUTING.md sentence that currently reads "在为 `jmcomic`
编写或改造代码时，请尽量增加类型注解 (Type hints) 并遵守合理的命名。除此之外，为确保全机队的统一性，请尽量遵循以下架构规范："; update
that exact string so it becomes "并遵守合理地命名" to correct the grammar.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test_api.ymlpyproject.tomlsetup.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- setup.py
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test_api.yml:
- Line 19: Update the GitHub Actions matrix for the python-version key by
removing '3.9' from the array (python-version: ['3.10','3.13','3.14']) to
reflect that 3.9 is EOL; if you intentionally must keep 3.9 as the supported
minimum, instead add an explicit inline comment next to the python-version array
explaining why 3.9 is retained and that it is tested intentionally despite being
EOL. Ensure you only modify the python-version array entry referenced in the
workflow.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CONTRIBUTING.md (1)
38-38:⚠️ Potential issue | 🟡 MinorBroken internal anchor on line 38 is still unresolved.
The link
#%EF%B8%8F-引擎改装规范-coding-guidelines(which decodes to only the variation-selector codepoint U+FE0F) does not match the anchor GitHub actually generates for## 🛠️ 引擎改装规范 (Coding Guidelines). GitHub strips emoji codepoints and turns adjacent spaces into hyphens, producing#-引擎改装规范-coding-guidelines. The markdownlint MD051 warning also confirms this.The most robust fix is an explicit HTML anchor:
🔗 Proposed fix
-在动工之前，请务必仔细阅读本文档末尾的《[🛠️ 引擎改装规范（Coding Guidelines）](#%EF%B8%8F-引擎改装规范-coding-guidelines)》 部分。 +在动工之前，请务必仔细阅读本文档末尾的《[🛠️ 引擎改装规范（Coding Guidelines）](`#coding-guidelines`)》 部分。And on the target heading:
-## 🛠️ 引擎改装规范 (Coding Guidelines) +## 🛠️ 引擎改装规范 (Coding Guidelines) <a id="coding-guidelines"></a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 38, The internal anchor link `#%EF%B8%8F-引擎改装规范-coding-guidelines` is broken; update the link and target by adding an explicit HTML anchor before the heading and point the link to that anchor: insert an ID like `<a id="coding-guidelines"></a>` immediately above the heading `## 🛠️ 引擎改装规范 (Coding Guidelines)` and change the link on line 38 to `#coding-guidelines` (or a clearly named anchor of your choice) so GitHub-generated anchors and markdownlint MD051 are avoided.
🧹 Nitpick comments (2)
src/jmcomic/jm_plugin.py (1)
297-298: Type mismatch:e(Exception) is passed asmsg(typedstr) — works at runtime but is type-unsafe.
jm_log('req.error', e)passes the exception as themsgparameter. This works becausedefault_jm_logginghas theisinstance(msg, BaseException)shorthand (jm_config.py line 29), but it silently violates the declaredmsg: strtype annotation onjm_log.Consider using the explicit three-argument form for clarity:
♻️ More explicit invocation
- jm_log('req.error', e) + jm_log('req.error', str(e), e)🤖 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 297 - 298, Replace the type-unsafe call jm_log('req.error', e) with the explicit three-argument form so the first arg stays the topic (string), the second is a string message (e.g., str(e)) and the third passes the exception object; update the call to jm_log('req.error', str(e), e) so it matches jm_log's typed signature and still provides the exception for default_jm_logging to inspect.src/jmcomic/jm_config.py (1)
27-36: Well-structured logging function with exception shorthand support.The
isinstance(msg, BaseException)shorthand and theexc_info=eusage are both correct (Python 3.5+ supports passing exception instances toexc_info).Minor nit from static analysis:
e: Exception = Noneshould bee: Optional[Exception] = Noneper PEP 484.♻️ Optional type annotation fix
-def default_jm_logging(topic: str, msg, e: Exception = None): +def default_jm_logging(topic: str, msg, e: Optional[Exception] = None):This requires importing
Optionalfromtyping(or usingException | Noneon Python 3.10+).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 27 - 36, Update the default_jm_logging signature to use an Optional type for the exception parameter: change "def default_jm_logging(topic: str, msg, e: Exception = None):" to "def default_jm_logging(topic: str, msg, e: Optional[Exception] = None):" (or "e: Exception | None = None" if the codebase targets Python 3.10+), and add the corresponding import "from typing import Optional" at the top of the module so the Optional annotation is defined; leave the body (isinstance(msg, BaseException) handling and jm_logger.error/jm_logger.info calls) unchanged.
🤖 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_config.py`:
- Around line 16-24: The Formatter is using the Python 3.10-only "defaults"
kwarg which breaks on 3.9; in setup_default_jm_logger remove the defaults
argument from logging.Formatter and instead create and add a logging.Filter
(e.g., subclass or lambda-style filter) that ensures every LogRecord has a topic
attribute (set record.topic = '' when missing) and attach that filter to the
handler (or jm_logger) before calling handler.setFormatter(...); keep
jm_logger.addHandler(handler) and jm_logger.setLevel(logging.INFO) as-is.
- Around line 407-410: The jm_log method now calls EXECUTOR_LOG(topic, msg, e)
which will break custom two-argument EXECUTOR_LOG implementations; update jm_log
to call EXECUTOR_LOG defensively by detecting or handling both signatures (e.g.,
inspect the callable signature or try calling with three args and fall back to
two args) so jm_log (and FLAG_ENABLE_JM_LOG gating) works with existing custom
loggers, referencing jm_log, EXECUTOR_LOG and FLAG_ENABLE_JM_LOG in your change;
alternatively document this as a breaking change in the release notes if you
choose not to add backward compatibility.
---
Duplicate comments:
In `@CONTRIBUTING.md`:
- Line 38: The internal anchor link `#%EF%B8%8F-引擎改装规范-coding-guidelines` is
broken; update the link and target by adding an explicit HTML anchor before the
heading and point the link to that anchor: insert an ID like `<a
id="coding-guidelines"></a>` immediately above the heading `## 🛠️ 引擎改装规范
(Coding Guidelines)` and change the link on line 38 to `#coding-guidelines` (or
a clearly named anchor of your choice) so GitHub-generated anchors and
markdownlint MD051 are avoided.
---
Nitpick comments:
In `@src/jmcomic/jm_config.py`:
- Around line 27-36: Update the default_jm_logging signature to use an Optional
type for the exception parameter: change "def default_jm_logging(topic: str,
msg, e: Exception = None):" to "def default_jm_logging(topic: str, msg, e:
Optional[Exception] = None):" (or "e: Exception | None = None" if the codebase
targets Python 3.10+), and add the corresponding import "from typing import
Optional" at the top of the module so the Optional annotation is defined; leave
the body (isinstance(msg, BaseException) handling and
jm_logger.error/jm_logger.info calls) unchanged.
In `@src/jmcomic/jm_plugin.py`:
- Around line 297-298: Replace the type-unsafe call jm_log('req.error', e) with
the explicit three-argument form so the first arg stays the topic (string), the
second is a string message (e.g., str(e)) and the third passes the exception
object; update the call to jm_log('req.error', str(e), e) so it matches jm_log's
typed signature and still provides the exception for default_jm_logging to
inspect.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/jmcomic/jm_config.py (2)
16-24: Missingtopicdefault on log records — directjm_loggercalls will produce stderr tracebacks.The
defaultskwarg was correctly removed (fixing the Python 3.10+ compatibility issue), but the suggested replacement — alogging.Filterthat injects a defaulttopicattribute — was not added. Any directjm_logger.info(...)call withoutextra={'topic': ...}will hit aKeyErrorin the formatter, causingHandler.handleError()to dump a traceback to stderr.Suggested fix — add a TopicFilter
def setup_default_jm_logger(): # 为了保持原有默认向下兼容，如果没有 handler，我们加一个控制台 handler if not jm_logger.handlers: import sys handler = logging.StreamHandler(sys.stdout) formatter = logging.Formatter('[%(asctime)s] [%(threadName)s]:【%(topic)s】%(message)s', datefmt='%Y-%m-%d %H:%M:%S') handler.setFormatter(formatter) jm_logger.addHandler(handler) jm_logger.setLevel(logging.INFO) + + class TopicFilter(logging.Filter): + def filter(self, record): + if not hasattr(record, 'topic'): + record.topic = '' + return True + + jm_logger.addFilter(TopicFilter())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 16 - 24, The formatter expects a record.topic attribute but direct jm_logger.info(...) calls can omit extra leading to KeyError; in setup_default_jm_logger() add a logging.Filter (e.g., TopicFilter) that ensures every LogRecord has a default record.topic (for example 'jm' or 'default') when missing and attach it to the handler via handler.addFilter(...) so the formatter never fails; reference setup_default_jm_logger, jm_logger, and the handler variable when locating where to register the filter.
407-410:LogTopicFilterPluginwill crash — its wrapper only accepts 2 args.This was flagged in a prior review, and the provided code snippets confirm the concrete breakage. In
jm_plugin.py(lines 557–561),LogTopicFilterPlugin.invoke()wrapsEXECUTOR_LOGwith:def new_jm_log(topic, msg): # only 2 params ... old_jm_log(topic, msg)Line 410 now calls
cls.EXECUTOR_LOG(topic, msg, e)with 3 arguments →TypeErrorat runtime whenever thelog_topic_filterplugin is active.At minimum, the plugin wrapper in
jm_plugin.pymust be updated to accept and forwarde:def new_jm_log(topic, msg, e=None): if whitelist is not None and topic not in whitelist: return old_jm_log(topic, msg, e)#!/bin/bash # Find all EXECUTOR_LOG wrappers or reassignments that only accept 2 args rg -nP 'EXECUTOR_LOG\s*=' --type=py -A5 echo "---" # Also check for any other custom jm_log replacements rg -nP 'def\s+\w*jm_log\w*\s*\(' --type=py -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 407 - 410, The LogTopicFilterPlugin wrapper for EXECUTOR_LOG only accepts two parameters causing a TypeError when jm_config.jm_log calls cls.EXECUTOR_LOG(topic, msg, e); update the wrapper in LogTopicFilterPlugin.invoke (the function named new_jm_log that replaces EXECUTOR_LOG / jm_log) to accept an optional third parameter (e=None) and forward it to the original old_jm_log (i.e., change its signature to accept topic, msg, e=None and call old_jm_log(topic, msg, e)); ensure any other custom EXECUTOR_LOG/jm_log replacements follow the same three-argument signature to avoid similar runtime errors.
🧹 Nitpick comments (1)
src/jmcomic/jm_config.py (1)
27-36: Solid shorthand for exception logging.The
isinstance(msg, BaseException)shorthand and theexc_info=eusage are both correct. One minor Ruff nit:Line 27:
e: Exception = Noneis an implicitOptional. Considere: Optional[Exception] = None(orException | Noneon 3.10+) per PEP 484.Same applies to line 408.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 27 - 36, Update the explicit exception type hints to be properly optional: replace e: Exception = None with either e: Optional[Exception] = None (and add from typing import Optional) or use Python 3.10 union syntax e: Exception | None = None; apply this change in default_jm_logging and the other function that currently declares e: Exception = None so both signatures correctly reflect an optional Exception type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/jmcomic/jm_config.py`:
- Around line 16-24: The formatter expects a record.topic attribute but direct
jm_logger.info(...) calls can omit extra leading to KeyError; in
setup_default_jm_logger() add a logging.Filter (e.g., TopicFilter) that ensures
every LogRecord has a default record.topic (for example 'jm' or 'default') when
missing and attach it to the handler via handler.addFilter(...) so the formatter
never fails; reference setup_default_jm_logger, jm_logger, and the handler
variable when locating where to register the filter.
- Around line 407-410: The LogTopicFilterPlugin wrapper for EXECUTOR_LOG only
accepts two parameters causing a TypeError when jm_config.jm_log calls
cls.EXECUTOR_LOG(topic, msg, e); update the wrapper in
LogTopicFilterPlugin.invoke (the function named new_jm_log that replaces
EXECUTOR_LOG / jm_log) to accept an optional third parameter (e=None) and
forward it to the original old_jm_log (i.e., change its signature to accept
topic, msg, e=None and call old_jm_log(topic, msg, e)); ensure any other custom
EXECUTOR_LOG/jm_log replacements follow the same three-argument signature to
avoid similar runtime errors.
---
Nitpick comments:
In `@src/jmcomic/jm_config.py`:
- Around line 27-36: Update the explicit exception type hints to be properly
optional: replace e: Exception = None with either e: Optional[Exception] = None
(and add from typing import Optional) or use Python 3.10 union syntax e:
Exception | None = None; apply this change in default_jm_logging and the other
function that currently declares e: Exception = None so both signatures
correctly reflect an optional Exception type.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/jmcomic/jm_config.py (1)
16-24: Filter for defaulttopicattribute is still missing — directjm_loggercalls will fail.The formatter at line 21 requires
%(topic)s, but no filter is added to inject a defaulttopicwhen users calljm_logger.info(...)directly withoutextra={'topic': ...}. Thedefaultsparameter was removed (correctly, since it requires Python 3.10+), but the suggested filter-based approach from the previous review was not implemented.Suggested fix — add a filter for default topic
def setup_default_jm_logger(): # 为了保持原有默认向下兼容，如果没有 handler，我们加一个控制台 handler if not jm_logger.handlers: import sys handler = logging.StreamHandler(sys.stdout) formatter = logging.Formatter('[%(asctime)s] [%(threadName)s]:【%(topic)s】%(message)s', datefmt='%Y-%m-%d %H:%M:%S') handler.setFormatter(formatter) jm_logger.addHandler(handler) jm_logger.setLevel(logging.INFO) + + # Ensure every record has a 'topic' attribute (compatible with Python 3.9+) + class TopicFilter(logging.Filter): + def filter(self, record): + if not hasattr(record, 'topic'): + record.topic = '' + return True + + jm_logger.addFilter(TopicFilter())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 16 - 24, The formatter in setup_default_jm_logger uses %(topic)s but no filter provides a default, so direct jm_logger.info(...) will KeyError; fix by creating and adding a logging.Filter (or a small subclass with a filter(self, record) method) that sets record.topic = getattr(record, "topic", "default") (or other sane default) and attach that filter to jm_logger (and/or the new handler) inside setup_default_jm_logger so every LogRecord has a topic before formatting; reference setup_default_jm_logger and jm_logger when locating where to add the filter.
🧹 Nitpick comments (1)
src/jmcomic/jm_config.py (1)
27-36: UseOptional[Exception]for PEP 484 compliance.The type hint
e: Exception = Noneimplicitly allowsNone, which PEP 484 prohibits. The same applies to line 408.Suggested fix
+from typing import Optional + -def default_jm_logging(topic: str, msg, e: Exception = None): +def default_jm_logging(topic: str, msg, e: Optional[Exception] = None):And at line 408:
- def jm_log(cls, topic: str, msg: str, e: Exception = None): + def jm_log(cls, topic: str, msg: str, e: Optional[Exception] = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 27 - 36, The parameter type hint e: Exception = None is not PEP 484 compliant; update default_jm_logging to use Optional[Exception] (e: Optional[Exception] = None) and import Optional from typing, and make the same change for the other function in this module that currently declares e: Exception = None so both annotations use Optional[Exception].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/jmcomic/jm_config.py`:
- Around line 16-24: The formatter in setup_default_jm_logger uses %(topic)s but
no filter provides a default, so direct jm_logger.info(...) will KeyError; fix
by creating and adding a logging.Filter (or a small subclass with a filter(self,
record) method) that sets record.topic = getattr(record, "topic", "default") (or
other sane default) and attach that filter to jm_logger (and/or the new handler)
inside setup_default_jm_logger so every LogRecord has a topic before formatting;
reference setup_default_jm_logger and jm_logger when locating where to add the
filter.
---
Nitpick comments:
In `@src/jmcomic/jm_config.py`:
- Around line 27-36: The parameter type hint e: Exception = None is not PEP 484
compliant; update default_jm_logging to use Optional[Exception] (e:
Optional[Exception] = None) and import Optional from typing, and make the same
change for the other function in this module that currently declares e:
Exception = None so both annotations use Optional[Exception].
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test_api.ymlsrc/jmcomic/jm_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test_api.yml
hect0x7
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/jmcomic/jm_config.py (1)
16-24: AI summary claimsTopicFilterwas introduced — it's not present in the code.The enriched summary states "Introduced module-level logger and TopicFilter," but no
TopicFilter(or any equivalentlogging.Filter) was added tosetup_default_jm_logger. The formatter at Line 21 still uses%(topic)swith no fallback. Any direct call tojm_logger.info/error()withoutextra={'topic': ...}will produce aKeyErrorsilently swallowed byHandler.handleError()and written tostderr— as originally flagged in the previous open review comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 16 - 24, The formatter in setup_default_jm_logger references %(topic)s but no TopicFilter or equivalent is added, causing KeyError when log calls omit extra['topic']; update setup_default_jm_logger to attach a logging.Filter (e.g., TopicFilter) to jm_logger or the handler that ensures a default 'topic' field (like empty string or "default") is present on the LogRecord if missing, and register that filter before adding the handler so jm_logger and its handler never emit records lacking 'topic'.
🧹 Nitpick comments (2)
src/jmcomic/jm_config.py (2)
408-413:try/except TypeErroris too broad — masks internalTypeErrors inEXECUTOR_LOG.If
EXECUTOR_LOG(topic, msg, e)raises aTypeErrorfrom within its own body (not from arity mismatch), theexceptblock silently swallows it and re-invokesEXECUTOR_LOG(topic, msg), causing double-invocation and hiding the real bug. The previous review comment proposed aninspect.signatureapproach that targets arity specifically; the current implementation chose the blunt try/except instead.Consider lazily checking arity once and caching the result instead of wrapping every call in a broad try/except:
♻️ Proposed refactor — lazy arity check
+ _executor_log_argc: int = None # cached arity of EXECUTOR_LOG + `@classmethod` def jm_log(cls, topic: str, msg: str, e: Exception = None): if cls.FLAG_ENABLE_JM_LOG is True: - try: - cls.EXECUTOR_LOG(topic, msg, e) - except TypeError: - cls.EXECUTOR_LOG(topic, msg) + if cls._executor_log_argc is None: + import inspect + try: + cls._executor_log_argc = len(inspect.signature(cls.EXECUTOR_LOG).parameters) + except (ValueError, TypeError): + cls._executor_log_argc = 3 + if cls._executor_log_argc >= 3: + cls.EXECUTOR_LOG(topic, msg, e) + else: + cls.EXECUTOR_LOG(topic, msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 408 - 413, jm_log currently uses a broad try/except around calling EXECUTOR_LOG(topic, msg, e) which can mask real TypeError inside EXECUTOR_LOG and cause double-invocation; change jm_log to determine once (lazily) whether EXECUTOR_LOG accepts three parameters by using inspect.signature or callable introspection on JMConfig.EXECUTOR_LOG, cache that boolean (e.g., _EXECUTOR_LOG_ACCEPTS_EXCEPTION) on the class, and then call either EXECUTOR_LOG(topic, msg, e) or EXECUTOR_LOG(topic, msg) based on the cached arity; keep honoring FLAG_ENABLE_JM_LOG and ensure the cached check runs only once to avoid per-call overhead.
27-27: UseOptional[Exception]instead of implicitOptionalon botheparameters.Both
default_jm_logging(Line 27) andJmModuleConfig.jm_log(Line 408) usee: Exception = None, which Ruff (RUF013) and PEP 484 flag as an implicitOptional.♻️ Proposed fix
-def default_jm_logging(topic: str, msg, e: Exception = None): +def default_jm_logging(topic: str, msg, e: Optional[Exception] = None):- def jm_log(cls, topic: str, msg: str, e: Exception = None): + def jm_log(cls, topic: str, msg: str, e: Optional[Exception] = None):Add
from typing import Optionalat the top of the file (or useException | Nonefor Python 3.10+ style).Also applies to: 408-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` at line 27, Update the implicit optional Exception annotations to explicit ones: add an import for Optional from typing (or use the Python 3.10 union syntax Exception | None) and change the function signature for default_jm_logging (the parameter e) and the jm_log member in JmModuleConfig to use Optional[Exception] (or Exception | None) instead of e: Exception = None so the types are explicit and satisfy Ruff/PEP484.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/jmcomic/jm_config.py`:
- Around line 16-24: The formatter in setup_default_jm_logger references
%(topic)s but no TopicFilter or equivalent is added, causing KeyError when log
calls omit extra['topic']; update setup_default_jm_logger to attach a
logging.Filter (e.g., TopicFilter) to jm_logger or the handler that ensures a
default 'topic' field (like empty string or "default") is present on the
LogRecord if missing, and register that filter before adding the handler so
jm_logger and its handler never emit records lacking 'topic'.
---
Nitpick comments:
In `@src/jmcomic/jm_config.py`:
- Around line 408-413: jm_log currently uses a broad try/except around calling
EXECUTOR_LOG(topic, msg, e) which can mask real TypeError inside EXECUTOR_LOG
and cause double-invocation; change jm_log to determine once (lazily) whether
EXECUTOR_LOG accepts three parameters by using inspect.signature or callable
introspection on JMConfig.EXECUTOR_LOG, cache that boolean (e.g.,
_EXECUTOR_LOG_ACCEPTS_EXCEPTION) on the class, and then call either
EXECUTOR_LOG(topic, msg, e) or EXECUTOR_LOG(topic, msg) based on the cached
arity; keep honoring FLAG_ENABLE_JM_LOG and ensure the cached check runs only
once to avoid per-call overhead.
- Line 27: Update the implicit optional Exception annotations to explicit ones:
add an import for Optional from typing (or use the Python 3.10 union syntax
Exception | None) and change the function signature for default_jm_logging (the
parameter e) and the jm_log member in JmModuleConfig to use Optional[Exception]
(or Exception | None) instead of e: Exception = None so the types are explicit
and satisfy Ruff/PEP484.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test_api.ymlsrc/jmcomic/jm_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test_api.yml
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/test_api.yml (1)
19-19: Python 3.14 addition is correct; consider plugging the 3.11/3.12 gap in the matrix.Python 3.14 was released on October 7, 2025 and is the latest stable release, so no
allow-prereleases: trueflag is needed — that part is correct.The inline comment documenting 3.9's intentional retention addresses the previous review.
However, the updated matrix skips Python 3.11 and 3.12 entirely. Both are still within their active or security support windows (3.11 until October 2027, 3.12 until October 2028) and represent a large portion of the real-world install base. A regression introduced between 3.10 and 3.13 could go undetected in CI.
🔧 Suggested matrix to plug the gap
- python-version: ['3.9', '3.10', '3.13', '3.14'] # 3.9 is kept as the minimum supported version test (EOL) + python-version: ['3.9', '3.10', '3.12', '3.13', '3.14'] # 3.9 is kept as the minimum supported version test (EOL)Adding 3.12 (as a representative mid-range version) keeps the matrix lean while covering the 3.10–3.13 gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test_api.yml at line 19, The matrix in the CI workflow omits Python 3.11/3.12, leaving a gap between 3.10 and 3.13; update the python-version entry for the test_api workflow (the "python-version" matrix key) to include a mid-range release (e.g., add '3.12' or both '3.11' and '3.12') so CI covers versions between 3.10 and 3.13 while keeping the matrix lean..github/CONTRIBUTING.md (1)
114-119:jm_loggeris undeclared in the bad-example snippet.The import block above (line 112) only brings in
jm_log, so the contrasting anti-pattern usingjm_logger.info(...)appears to reference an object that has never been obtained. A reader may wonder whetherjm_loggeris an importable symbol fromjmcomic. A one-line context comment (e.g.,# jm_logger = logging.getLogger('jmcomic')) would make the anti-pattern immediately self-explanatory.📝 Proposed clarification
-# ❌ 避免直接调用底层 logger -jm_logger.info("【api】请求成功") +# ❌ 避免直接调用底层 logger（jm_logger 是 jmcomic 内部的原始 logging.Logger 实例） +jm_logger.info("【api】请求成功")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/CONTRIBUTING.md around lines 114 - 119, The bad-example uses an undeclared symbol jm_logger which confuses readers; update the snippet to either declare or clarify jm_logger by adding a one-line context comment explaining how it would be obtained (e.g., that jm_logger would be created via the standard logging API) or replace the bad-example with an explicitly declared logger, and keep the recommended jm_log('api', ...) example for contrast; reference the symbols jm_logger and jm_log so reviewers can locate and fix the snippet.src/jmcomic/jm_config.py (1)
27-27:Optional[Exception]annotation preferred over implicitOptionalPEP 484 prohibits
e: Exception = None; useOptional[Exception]explicitly (Ruff RUF013). Same applies to line 408.🔧 Proposed fix
-def default_jm_logging(topic: str, msg, e: Exception = None): +def default_jm_logging(topic: str, msg, e: Optional[Exception] = None):+from typing import Optional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` at line 27, The type annotation for the exception parameter in default_jm_logging should be explicit per PEP 484: change the signature from e: Exception = None to e: Optional[Exception] = None and add "from typing import Optional" at the top of the module; apply the same change to the other function referenced around line 408 (update its exception parameter to Optional[Exception] = None and ensure Optional is imported so both annotations are valid).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/CONTRIBUTING.md:
- Around line 114-119: The bad-example uses an undeclared symbol jm_logger which
confuses readers; update the snippet to either declare or clarify jm_logger by
adding a one-line context comment explaining how it would be obtained (e.g.,
that jm_logger would be created via the standard logging API) or replace the
bad-example with an explicitly declared logger, and keep the recommended
jm_log('api', ...) example for contrast; reference the symbols jm_logger and
jm_log so reviewers can locate and fix the snippet.
In @.github/workflows/test_api.yml:
- Line 19: The matrix in the CI workflow omits Python 3.11/3.12, leaving a gap
between 3.10 and 3.13; update the python-version entry for the test_api workflow
(the "python-version" matrix key) to include a mid-range release (e.g., add
'3.12' or both '3.11' and '3.12') so CI covers versions between 3.10 and 3.13
while keeping the matrix lean.
In `@src/jmcomic/jm_config.py`:
- Line 27: The type annotation for the exception parameter in default_jm_logging
should be explicit per PEP 484: change the signature from e: Exception = None to
e: Optional[Exception] = None and add "from typing import Optional" at the top
of the module; apply the same change to the other function referenced around
line 408 (update its exception parameter to Optional[Exception] = None and
ensure Optional is imported so both annotations are valid).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/CONTRIBUTING.md.github/workflows/test_api.ymlREADME.mdassets/readme/README-en.mdassets/readme/README-jp.mdassets/readme/README-kr.mdsrc/jmcomic/jm_config.py
🚧 Files skipped from review as they are similar to previous changes (4)
- assets/readme/README-kr.md
- README.md
- assets/readme/README-jp.md
- assets/readme/README-en.md
hect0x7
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/CONTRIBUTING.md (1)
38-38: Broken anchor — MD051 still flagged by markdownlint.The static analysis tool continues to flag this link fragment as invalid. The author has self-tested and confirmed it works on GitHub, so no change is required if that holds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/CONTRIBUTING.md at line 38, MD051 is flagging the link fragment for "🛠️ 引擎改装规范（Coding Guidelines）" as broken; verify and fix the anchor so markdownlint stops complaining by making the link fragment exactly match the generated header id or by adding an explicit HTML id on the target header. Specifically check the header text "🛠️ 引擎改装规范（Coding Guidelines）" and either update the link fragment "#%EF%B8%8F-引擎改装规范-coding-guidelines" to the exact slug produced by your Markdown renderer (encode emojis/parentheses appropriately) or add an explicit id attribute to that header (e.g., <h2 id="engine-coding-guidelines">) and point the link to that id; confirm it still renders correctly on GitHub and that markdownlint no longer flags MD051.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/CONTRIBUTING.md:
- Line 38: MD051 is flagging the link fragment for "🛠️ 引擎改装规范（Coding
Guidelines）" as broken; verify and fix the anchor so markdownlint stops
complaining by making the link fragment exactly match the generated header id or
by adding an explicit HTML id on the target header. Specifically check the
header text "🛠️ 引擎改装规范（Coding Guidelines）" and either update the link fragment
"#%EF%B8%8F-引擎改装规范-coding-guidelines" to the exact slug produced by your
Markdown renderer (encode emojis/parentheses appropriately) or add an explicit
id attribute to that header (e.g., <h2 id="engine-coding-guidelines">) and point
the link to that id; confirm it still renders correctly on GitHub and that
markdownlint no longer flags MD051.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/jmcomic/jm_config.py (1)
16-24:⚠️ Potential issue | 🟡 Minor
%(topic)sin formatter still has no fallback — directjm_loggercalls withoutextrawill silently error.The format string at line 21 references
%(topic)s, but neither adefaultsmapping nor alogging.Filteris present to supply it. Any code that callsjm_logger.info/debug/warning()directly (withoutextra={'topic': ...}) triggers aKeyErrorcaught byHandler.handleError(), which writes a traceback tostderr. This was identified in prior reviews; thedefaults={'topic': ''}fix was reverted for Python 3.9 compat, but the recommended filter-based alternative was not applied.A
logging.Filteronjm_loggerworks on Python 3.9+:🛡️ Proposed fix — inject a default topic via Filter (Python 3.9 compatible)
def setup_default_jm_logger(): # 为了保持原有默认向下兼容，如果没有 handler，我们加一个控制台 handler if not jm_logger.handlers: import sys handler = logging.StreamHandler(sys.stdout) formatter = logging.Formatter('[%(asctime)s] [%(threadName)s]:【%(topic)s】%(message)s', datefmt='%Y-%m-%d %H:%M:%S') handler.setFormatter(formatter) jm_logger.addHandler(handler) jm_logger.setLevel(logging.INFO) + + class _DefaultTopic(logging.Filter): + def filter(self, record): + if not hasattr(record, 'topic'): + record.topic = '' + return True + + if not any(isinstance(f, _DefaultTopic) for f in jm_logger.filters): + jm_logger.addFilter(_DefaultTopic())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 16 - 24, The formatter references %(topic)s but no default is provided, causing KeyError when callers log without extra; inside setup_default_jm_logger add and attach a logging.Filter (e.g., a small class or lambda-style filter) to jm_logger or the handler that ensures record.topic exists (set to empty string or a sensible default) before formatting, then keep the existing formatter/handler setup and jm_logger.setLevel call so direct jm_logger.info/debug/warning() calls no longer raise via Handler.handleError().
🧹 Nitpick comments (2)
src/jmcomic/jm_config.py (2)
27-27: PEP 484 prohibits implicitOptional— applies to bothdefault_jm_logging(line 27) andjm_log(line 408).Both
e: Exception = Nonesignatures should use an explicitOptional:🔧 Suggested fix
+from typing import Optional + -def default_jm_logging(topic: str, msg, e: Exception = None): +def default_jm_logging(topic: str, msg, e: Optional[Exception] = None):- def jm_log(cls, topic: str, msg: str, e: Exception = None): + def jm_log(cls, topic: str, msg: str, e: Optional[Exception] = None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` at line 27, Update the type annotations for the optional exception parameters to use typing.Optional instead of implicit None: change the signatures of default_jm_logging and jm_log to accept e: Optional[Exception] = None, add "from typing import Optional" at the top of the module (or include Optional in the existing typing import), and run a quick type-check to ensure no other annotations rely on implicit Optional usage.
16-24: Library adds aStreamHandlerat import time — duplicate output when the application configures the root logger.
jm_logger.propagatedefaults toTrue. If the application callslogging.basicConfig()or adds any root-logger handler after (or before) importingjmcomic, everyjmcomiclog record is emitted twice: once throughjm_logger's ownStreamHandlerand once through propagation to the root logger. The Python logging documentation recommends library authors useNullHandlerto avoid interfering with the application's logging configuration.Since this is intentionally for backward compatibility, a lighter mitigation is to set
jm_logger.propagate = Falseinsidesetup_default_jm_loggerwhen the default handler is added — preventing root-logger duplication while keeping the backward-compat output path intact.💡 Suggested mitigation
if not jm_logger.handlers: import sys handler = logging.StreamHandler(sys.stdout) formatter = logging.Formatter(...) handler.setFormatter(formatter) jm_logger.addHandler(handler) jm_logger.setLevel(logging.INFO) + jm_logger.propagate = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 16 - 24, The setup_default_jm_logger function currently adds a StreamHandler which causes duplicate logs via propagation; modify setup_default_jm_logger so that when you add the default StreamHandler to jm_logger you also set jm_logger.propagate = False (only in the branch that adds the handler) to prevent records from also being handled by the root logger while preserving the existing console output fallback behavior.
🤖 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_config.py`:
- Around line 408-426: The jm_log exception path currently calls
inspect.signature(executor) on every error and emits a warnings.warn without
stacklevel; change jm_log to cache the executor arity (e.g., a class-level dict
keyed by id(EXECUTOR_LOG) or executor object to store params_count) so you only
compute inspect.signature once per executor identity, and when emitting the
deprecation/warning call inside jm_log include stacklevel=2 in warnings.warn so
the warning points to the caller site; update code references jm_log,
EXECUTOR_LOG and FLAG_ENABLE_JM_LOG to use the cached arity and add stacklevel=2
to the warning invocation.
---
Duplicate comments:
In `@src/jmcomic/jm_config.py`:
- Around line 16-24: The formatter references %(topic)s but no default is
provided, causing KeyError when callers log without extra; inside
setup_default_jm_logger add and attach a logging.Filter (e.g., a small class or
lambda-style filter) to jm_logger or the handler that ensures record.topic
exists (set to empty string or a sensible default) before formatting, then keep
the existing formatter/handler setup and jm_logger.setLevel call so direct
jm_logger.info/debug/warning() calls no longer raise via Handler.handleError().
---
Nitpick comments:
In `@src/jmcomic/jm_config.py`:
- Line 27: Update the type annotations for the optional exception parameters to
use typing.Optional instead of implicit None: change the signatures of
default_jm_logging and jm_log to accept e: Optional[Exception] = None, add "from
typing import Optional" at the top of the module (or include Optional in the
existing typing import), and run a quick type-check to ensure no other
annotations rely on implicit Optional usage.
- Around line 16-24: The setup_default_jm_logger function currently adds a
StreamHandler which causes duplicate logs via propagation; modify
setup_default_jm_logger so that when you add the default StreamHandler to
jm_logger you also set jm_logger.propagate = False (only in the branch that adds
the handler) to prevent records from also being handled by the root logger while
preserving the existing console output fallback behavior.
| def jm_log(cls, topic: str, msg: str, e: Exception = None): | ||
| if cls.FLAG_ENABLE_JM_LOG is True: | ||
| cls.EXECUTOR_LOG(topic, msg) | ||
| executor = cls.EXECUTOR_LOG | ||
| if e is None: | ||
| executor(topic, msg) | ||
| else: | ||
| import inspect | ||
| try: | ||
| sig = inspect.signature(executor) | ||
| params_count = len(sig.parameters) | ||
| except (ValueError, TypeError): | ||
| params_count = 2 | ||
|
|
||
| if params_count >= 3: | ||
| executor(topic, msg, e) | ||
| else: | ||
| import warnings | ||
| warnings.warn('jmcomic已升级到标准logging，建议将EXECUTOR_LOG重新定义为3个参数以接收异常对象 (topic, msg, e)') | ||
| executor(topic, msg) |
There was a problem hiding this comment.
warnings.warn missing stacklevel + repeated inspect.signature reflection on every error call.
Two issues in the exception dispatch path:
-
stacklevelabsent (Ruff B028, line 425): Withoutstacklevel=2, the warning's source location points insidejm_lograther than the caller site, making it hard for users to identify which code needs updating. -
Uncached arity detection (lines 414–419):
inspect.signature(executor)is invoked on everyjm_logcall that includes an exception.EXECUTOR_LOGrarely changes at runtime, so caching the result (keyed by executor identity) avoids repeated reflection overhead in error-heavy paths.
✨ Proposed fix
+ _executor_arity_cache: dict = {}
+
`@classmethod`
def jm_log(cls, topic: str, msg: str, e: Exception = None):
if cls.FLAG_ENABLE_JM_LOG is True:
executor = cls.EXECUTOR_LOG
if e is None:
executor(topic, msg)
else:
- import inspect
- try:
- sig = inspect.signature(executor)
- params_count = len(sig.parameters)
- except (ValueError, TypeError):
- params_count = 2
+ cache = cls._executor_arity_cache
+ if executor not in cache:
+ import inspect
+ try:
+ cache[executor] = len(inspect.signature(executor).parameters)
+ except (ValueError, TypeError):
+ cache[executor] = 2
+ params_count = cache[executor]
if params_count >= 3:
executor(topic, msg, e)
else:
import warnings
- warnings.warn('jmcomic已升级到标准logging，建议将EXECUTOR_LOG重新定义为3个参数以接收异常对象 (topic, msg, e)')
+ warnings.warn(
+ 'jmcomic已升级到标准logging，建议将EXECUTOR_LOG重新定义为3个参数以接收异常对象 (topic, msg, e)',
+ stacklevel=2,
+ )
executor(topic, msg)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 408-408: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
[warning] 425-425: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
[warning] 425-425: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/jmcomic/jm_config.py` around lines 408 - 426, The jm_log exception path
currently calls inspect.signature(executor) on every error and emits a
warnings.warn without stacklevel; change jm_log to cache the executor arity
(e.g., a class-level dict keyed by id(EXECUTOR_LOG) or executor object to store
params_count) so you only compute inspect.signature once per executor identity,
and when emitting the deprecation/warning call inside jm_log include
stacklevel=2 in warnings.warn so the warning points to the caller site; update
code references jm_log, EXECUTOR_LOG and FLAG_ENABLE_JM_LOG to use the cached
arity and add stacklevel=2 to the warning invocation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/docs/sources/tutorial/11_log_custom.md`:
- Around line 11-18: Split the combined example into two separate, clearly
labeled code blocks so readers understand they are alternatives: create one
block titled "方式 A（推荐）：使用 Python 原生 logging 控制" showing use of
logging.getLogger("jmcomic").setLevel(logging.WARNING), and a second block
titled "方式 B：使用 disable_jm_log() 完全关闭日志" showing from jmcomic import
disable_jm_log and disable_jm_log(). Ensure the text explains they are mutually
exclusive alternatives and remove the single sequential snippet that mixes both.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/docs/sources/tutorial/11_log_custom.mdassets/docs/sources/tutorial/4_module_custom.md
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/docs/sources/tutorial/4_module_custom.md
| ```python | ||
| from jmcomic import disable_jm_log | ||
| disable_jm_log() | ||
|
|
||
| # 推荐使用 Python 原生的 logging 控制 | ||
| import logging | ||
| logging.getLogger("jmcomic").setLevel(logging.WARNING) # 只显示警告以上的日志 | ||
| ``` |
There was a problem hiding this comment.
Split the two alternative approaches into separate, clearly labelled code blocks.
disable_jm_log() and logging.getLogger(...).setLevel(...) are mutually exclusive alternatives, but they are presented as a single sequential snippet. A reader who copies the block calls disable_jm_log() first (which disables all output) and then calls setLevel(logging.WARNING) — the outcome depends on how disable_jm_log is implemented and is at best confusing, at worst a silent no-op for the second call. The inline comment "推荐使用 Python 原生的 logging 控制" already signals that the second snippet is preferred, so presenting both in one block contradicts that recommendation.
📝 Proposed fix: separate into two distinct blocks
-```python
-from jmcomic import disable_jm_log
-disable_jm_log()
-
-# 推荐使用 Python 原生的 logging 控制
-import logging
-logging.getLogger("jmcomic").setLevel(logging.WARNING) # 只显示警告以上的日志
-```
+**方式 A（推荐）：使用 Python 原生 logging 控制**
+
+```python
+import logging
+logging.getLogger("jmcomic").setLevel(logging.WARNING) # 只显示警告以上的日志
+```
+
+**方式 B：使用 `disable_jm_log()` 完全关闭日志**
+
+```python
+from jmcomic import disable_jm_log
+disable_jm_log()
+```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/docs/sources/tutorial/11_log_custom.md` around lines 11 - 18, Split
the combined example into two separate, clearly labeled code blocks so readers
understand they are alternatives: create one block titled "方式 A（推荐）：使用 Python 原生
logging 控制" showing use of
logging.getLogger("jmcomic").setLevel(logging.WARNING), and a second block
titled "方式 B：使用 disable_jm_log() 完全关闭日志" showing from jmcomic import
disable_jm_log and disable_jm_log(). Ensure the text explains they are mutually
exclusive alternatives and remove the single sequential snippet that mixes both.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/jmcomic/jm_config.py (2)
16-24:%(topic)sin formatter still lacks a fallback afterdefaultsrevert — directjm_loggercalls will error.The
defaultskwarg was correctly reverted for Python 3.9 compatibility, but the Filter-based alternative from that same review was never applied. Any directjm_logger.info("msg")call (withoutextra={'topic': ...}) hits aKeyErrorin the formatter, whichHandler.handleErrorcatches and dumps to stderr.Add a
logging.Filterto inject a defaulttopicattribute:Proposed fix
def setup_default_jm_logger(): # 为了保持原有默认向下兼容，如果没有 handler，我们加一个控制台 handler if not jm_logger.handlers: import sys handler = logging.StreamHandler(sys.stdout) formatter = logging.Formatter('[%(asctime)s] [%(threadName)s]:【%(topic)s】%(message)s', datefmt='%Y-%m-%d %H:%M:%S') handler.setFormatter(formatter) jm_logger.addHandler(handler) jm_logger.setLevel(logging.INFO) + + # Inject default 'topic' for records that omit extra={'topic': ...} (Python 3.9-safe) + class _TopicDefault(logging.Filter): + def filter(self, record): + if not hasattr(record, 'topic'): + record.topic = '' + return True + + jm_logger.addFilter(_TopicDefault())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 16 - 24, The formatter references record.topic but direct jm_logger calls without extra={'topic':...} raise KeyError; update setup_default_jm_logger to attach a logging.Filter (e.g., subclass Filter with filter(self, record): if not hasattr(record,'topic'): record.topic='JM') to the handler or jm_logger so a default topic is injected before formatting; locate setup_default_jm_logger and jm_logger and add/apply that Filter to prevent KeyError when extra is missing.
408-429: Backward-compat dispatch logic is correct;stacklevel=2addressed.The arity detection via
inspect.signaturecorrectly handles plain functions, lambdas, bound methods, and built-ins (via theexceptfallback). Thestacklevel=2addition ensures the warning points to the caller site.The uncached
inspect.signaturecall on every exception-bearingjm_loginvocation was flagged in a prior review. It's a minor performance concern on error-heavy paths — consider caching keyed onid(executor)if this becomes hot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 408 - 429, The jm_log function repeatedly calls inspect.signature(executor) whenever an exception is logged, which can be expensive; cache the arity detection keyed by id(executor) (e.g., a module-level dict mapping executor id to parameter count) and use that cached params_count instead of recomputing on each invocation; update jm_log (and any helper that computes params_count) to consult the cache, populate it in the except/try flow when needed, and keep the existing fallback and warning behavior intact; refer to jm_log, EXECUTOR_LOG and FLAG_ENABLE_JM_LOG to locate where to add the cache.
🧹 Nitpick comments (1)
src/jmcomic/jm_config.py (1)
27-36: Solid shorthand support and clean logging dispatch.The
isinstance(msg, BaseException)check forjm_log('topic', e)shorthand is a nice convenience. Logging withexc_info=efor exceptions is correct.Nit: Ruff RUF013 flags
e: Exception = None— PEP 484 prefersOptional[Exception](orException | Nonefor ≥3.10). Same applies tojm_logat Line 408.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jmcomic/jm_config.py` around lines 27 - 36, Update the function signatures to use an explicit optional type for the exception parameter (e.g., change e: Exception = None to e: Optional[Exception] or Exception | None) in both default_jm_logging and the jm_log function to satisfy PEP 484/ruff RUF013; add the necessary typing import (from typing import Optional) if using Optional, and ensure any type hints and docstrings are adjusted accordingly.
🤖 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_config.py`:
- Around line 410-429: The LogTopicFilterPlugin is replacing EXECUTOR_LOG with
new_jm_log(topic, msg) which drops exceptions; change
LogTopicFilterPlugin.new_jm_log to accept an optional third parameter (e=None)
and forward it to the previous executor (call old_jm_log(topic, msg, e)) so
exceptions aren’t silently discarded; update any closures that wrap jm_log/_log
to use a (topic, msg, e=None) signature and ensure callers of EXECUTOR_LOG
continue to work with 3-arg executors while still supporting legacy 2-arg
handlers if needed.
---
Duplicate comments:
In `@src/jmcomic/jm_config.py`:
- Around line 16-24: The formatter references record.topic but direct jm_logger
calls without extra={'topic':...} raise KeyError; update setup_default_jm_logger
to attach a logging.Filter (e.g., subclass Filter with filter(self, record): if
not hasattr(record,'topic'): record.topic='JM') to the handler or jm_logger so a
default topic is injected before formatting; locate setup_default_jm_logger and
jm_logger and add/apply that Filter to prevent KeyError when extra is missing.
- Around line 408-429: The jm_log function repeatedly calls
inspect.signature(executor) whenever an exception is logged, which can be
expensive; cache the arity detection keyed by id(executor) (e.g., a module-level
dict mapping executor id to parameter count) and use that cached params_count
instead of recomputing on each invocation; update jm_log (and any helper that
computes params_count) to consult the cache, populate it in the except/try flow
when needed, and keep the existing fallback and warning behavior intact; refer
to jm_log, EXECUTOR_LOG and FLAG_ENABLE_JM_LOG to locate where to add the cache.
---
Nitpick comments:
In `@src/jmcomic/jm_config.py`:
- Around line 27-36: Update the function signatures to use an explicit optional
type for the exception parameter (e.g., change e: Exception = None to e:
Optional[Exception] or Exception | None) in both default_jm_logging and the
jm_log function to satisfy PEP 484/ruff RUF013; add the necessary typing import
(from typing import Optional) if using Optional, and ensure any type hints and
docstrings are adjusted accordingly.
| executor = cls.EXECUTOR_LOG | ||
| if e is None: | ||
| executor(topic, msg) | ||
| else: | ||
| import inspect | ||
| try: | ||
| sig = inspect.signature(executor) | ||
| params_count = len(sig.parameters) | ||
| except (ValueError, TypeError): | ||
| params_count = 2 | ||
|
|
||
| if params_count >= 3: | ||
| executor(topic, msg, e) | ||
| else: | ||
| import warnings | ||
| warnings.warn( | ||
| 'jmcomic已升级到标准logging，建议将EXECUTOR_LOG重新定义为3个参数以接收异常对象 (topic, msg, e)', | ||
| stacklevel=2 | ||
| ) | ||
| executor(topic, msg) |
There was a problem hiding this comment.
LogTopicFilterPlugin will silently drop exceptions — consider updating it.
The LogTopicFilterPlugin (in jm_plugin.py:547-562) defines new_jm_log(topic, msg) with only 2 parameters and replaces EXECUTOR_LOG. When jm_log is called with an exception, the arity check here will detect 2 params, emit the deprecation warning, and call with (topic, msg) — silently discarding the exception object even though the underlying executor (old_jm_log) supports 3 params.
Consider updating LogTopicFilterPlugin.new_jm_log to (topic, msg, e=None) and forwarding e to old_jm_log.
#!/bin/bash
# Verify all EXECUTOR_LOG replacements in the codebase that use 2-param signatures
rg -n "EXECUTOR_LOG\s*=" --type=py -B1 -A5
echo "---"
# Also check for any closures wrapping the log executor
rg -n "def.*jm_log\|def.*_log" --type=py -B1 -A3 | grep -A4 "def "🧰 Tools
🪛 Ruff (0.15.1)
[warning] 426-426: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/jmcomic/jm_config.py` around lines 410 - 429, The LogTopicFilterPlugin is
replacing EXECUTOR_LOG with new_jm_log(topic, msg) which drops exceptions;
change LogTopicFilterPlugin.new_jm_log to accept an optional third parameter
(e=None) and forward it to the previous executor (call old_jm_log(topic, msg,
e)) so exceptions aren’t silently discarded; update any closures that wrap
jm_log/_log to use a (topic, msg, e=None) signature and ensure callers of
EXECUTOR_LOG continue to work with 3-arg executors while still supporting legacy
2-arg handlers if needed.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores