fix(asr): align iFLYTEK integration for 0.2.1 - #2308
dillonliang224 wants to merge 1 commit into
Conversation
|
Reviewed the full diff plus the surrounding package ( What is correct The rename is complete. All 10 references to Line-length discipline holds. CI enforces
Docs version bumps are correctly scoped. Main observation: the lint suppression targets a linter this repo does not run The
The same reasoning applies to the rename itself, which reads as satisfying the ruff rule Relatedly, the changelog says the change "document[s] the intentional lazy extension import", but a bare suppression code is not documentation. If the intent is to explain WHY the import is deferred (import cost, or circularity at addon registration), a short prose comment carries that; the code alone does not. Minor
Test coverage, performance, security Test changes are the correct mechanical follow-through. The
No performance implications. No security surface touched — no credential handling, logging, or redaction logic changed. Verdict Good to merge once the PLC0415 suppression question is settled. My preference is to drop it (it is inert under the actual lint config of this repo, and inconsistent with sibling addons), or to pair it with a real ruff config. Everything else in the patch is clean and correct. |
dec4da4 to
b3ee178
Compare
Review:
|
b3ee178 to
06327dc
Compare
Review:
|
06327dc to
d212914
Compare
|
Tight, well-scoped release PR. The rename is applied consistently, the alias keeps 0.2.x callers working, and adding a version-parity test is a real improvement over a manual sync step. One issue is worth fixing before merge; the rest are minor. Main issue: the tomllib fallback depends on an undeclared package try:
import tomllib
except ModuleNotFoundError:
import tomli as tomllib
It may resolve today because pytest has historically carried a Three ways out, cheapest first:
import re
text = (PACKAGE_ROOT / "pyproject.toml").read_text(encoding="utf-8")
match = re.search(r"^version\s*=\s*\"([^\"]+)\"", text, re.MULTILINE)
assert match is not None, "pyproject.toml is missing a version"
assert manifest["version"] == match.group(1)The
I would take option 1. The deprecated alias is not actually deprecated ReconnectLimitReached = ReconnectLimitReachedErrorThe CHANGELOG calls this a deprecated alias, but nothing warns, so a 0.2.x caller gets no signal before the 0.3.0 removal. They will just break. If the deprecation should carry weight, a module-level Separately, worth asking whether the alias is needed at all. I grepped for Smaller notes
Conventions Commit message follows the Test coverage Reasonable for a patch release: the rename is exercised through Nothing here is a security or performance concern: no I/O, auth, or hot-path changes. |
d212914 to
10ef597
Compare
|
Review of Focused, well-scoped change. The rename is applied consistently, the compatibility alias is the right mechanism, and adding a version-drift test alongside the version bump is a good instinct. A few things worth addressing before merge. ISSUES
In
This is correct today —
SMALLER POINTS Compatibility alias. Alias test. Naming. The Semver. Renaming a public exception is technically breaking; the alias is what makes 0.2.1 defensible rather than 0.3.0. Flagging that the alias is load-bearing for the patch-level choice, which is another reason not to drop it early. collections.abc.Mapping. Correct change — COVERAGE, PERFORMANCE, SECURITY Test coverage is adequate for the change: the renamed exception is exercised on both the Nothing here is a blocker beyond the encoding fix, which is a one-line change. |
Align reconnect error naming and Python compatibility with the reviewed conversational agent integration. Update extension metadata, release documentation, and affected tests for the 0.2.1 package.
10ef597 to
078cae2
Compare
|
Review of fix(asr): align iFLYTEK integration for 0.2.1 Basis: I read the full diff plus the surrounding code (extension.py, reconnect_manager.py, protocol.py, the package tests, and the docs/README set). Lint and test tooling was not runnable in my environment, so everything below comes from static reading rather than from executing the suite. Overall this is a clean, well-scoped patch release. The rename is mechanical and complete, and the new package-contract test guards a real class of drift. WHAT LOOKS CORRECT
MAIN DISCUSSION POINT: IS THE LEGACY ALIAS NEEDED? I grepped for importers of ReconnectLimitReached. The only ones are extension.py and this package own tests, all updated in this PR. init.py only does "from . import addon", so the exception was never part of the package export surface, and TEN extensions load through the addon registry rather than being imported as a library by third parties. So there does not appear to be a 0.2.x caller that the alias protects. Given that 04_conventions.md calls out YAGNI explicitly, I would lean toward dropping the alias along with test_legacy_reconnect_limit_exception_name_is_preserved, and letting the rename stand on its own. That also removes the 0.3.0 cleanup obligation: the comment is the only thing tracking it today, and nothing fails if it is forgotten. If you are keeping it because something out of tree imports it, that is a fair call, but it would help to note where. In that case the current form (a plain identity alias, so "except ReconnectLimitReached" still catches) is the right implementation. NOTES ON THE NEW CONTRACT TEST
DOCS The README files under docs/ and the top-level README.md still say 0.2.0, but those are historical statements about when params nesting became mandatory, so they should not be bumped. PRODUCTION_READINESS.md is the only file carrying a "this checklist applies to version X" stamp, and that is the one updated. Correct as-is; flagging it so a later reviewer does not try to fix it. BEFORE MERGE Per 04_conventions.md, task lint is strict (a single warning is fatal) and the format and lint hooks do not run for a commit made from a host shell. Worth confirming: One specific thing to watch: pylint const-naming-style=UPPER_CASE against the module-level ReconnectLimitReached assignment. Pylint normally infers a class alias and applies class-naming-style (PascalCase, which passes), but that is worth confirming rather than assuming, and it becomes moot if the alias is dropped. Also worth running the package suite directly, since tests/ is excluded from pylint and the new contract test is therefore not exercised by lint: Commit message and branch name both follow the conventions (fix(asr): prefix, fix/ branch, body wrapped under 100 columns), so no commitlint risk that I can see. Nothing here blocks except the alias question, which is a design call rather than a defect. |
Align reconnect error naming and Python compatibility with the reviewed conversational agent integration. Update extension metadata, release documentation, and affected tests for the 0.2.1 package.