chore: code-review quick-wins cleanup (core, scripts, app-runner)#557
Draft
leoschwarz wants to merge 5 commits into
Draft
chore: code-review quick-wins cleanup (core, scripts, app-runner)#557leoschwarz wants to merge 5 commits into
leoschwarz wants to merge 5 commits into
Conversation
Reject non-SUDS engines up front in parse_method_signature with a clear RuntimeError instead of leaking an AttributeError from private engine internals when running 'api inspect' under the Zeep engine.
… banner Relocate DEFAULT_THEME and HostnameHighlighter from the CLI-presentation module utils/cli_integration.py to a neutral utils/console.py, fixing the core-imports-CLI layering inversion in bfabric.py; cli_integration keeps a back-compat re-export. Demote the per-construction version banner from INFO to DEBUG so library/server/test contexts stay quiet by default.
_uv_bin was annotated -> str but returned shutil.which("uv") (str | None),
so a missing uv produced an obscure TypeError downstream. Raise a clear
RuntimeError instead, making the return type honest and removing the
grandfathered reportReturnType baseline entry.
…urface Replace the hand-maintained issubclass dispatch ladder in the input Resolver with a dict[type, resolver] registry, keeping an MRO-walk fallback (so a future BfabricAnnotationSpec union still routes) and adding an import-time exhaustiveness check against InputSpecType. This eliminates the parallel __init__/ladder boilerplate (and 38 grandfathered reportUnknown baseline entries). Rename bfabric_resource_dataset.py -> bfabric_resource_dataset_spec.py to match its siblings' _spec suffix. Also delete dead spec surface: the four uncalled resolve_filename methods and the entirely-unused BfabricAnnotationSpecField. The BfabricAnnotationSpec union-of-one alias is intentionally kept for the MRO-walk scaffolding.
Supersedes the earlier INFO->DEBUG banner tweak, which did not actually
quiet the banner: loguru's default handler emits DEBUG too, so the banner
still printed in unconfigured library/server/test contexts.
Instead follow loguru's library convention: bfabric calls logger.disable
("bfabric") at import and the CLI re-enables it in setup_script_logging.
The version banner stays a loguru record at INFO; theme/highlighter stay
in cli_integration (no new module).
leoschwarz
force-pushed
the
chore/code-review-quick-wins
branch
from
July 16, 2026 06:47
5ec2967 to
65b5690
Compare
Contributor
📝 "TODO" Changes DetectedSummary: ✅ 1 "TODO" removed ✅ Removed "TODO"s (1)
This comment is automatically updated when "TODO" changes are detected. |
leoschwarz
changed the base branch from
main
to
refactor/app-runner-findmixin-migration
July 16, 2026 06:47
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.
Groups six small, independent cleanups surfaced by a code review. All behavior-preserving except the logging change noted below; none touch the core read/write API.
Changes
api inspectnow fails with a clear "only supported with the SUDS engine" error instead of leaking anAttributeErrorfrom private engine internals under Zeep.Entity.ENDPOINTbecomes aClassVar[str]attribute instead of a deprecated, unused instance method that contradicted thestrevery subclass andFindMixinalready assumed — clearing thereportAssignmentTypesuppressions this forced across ~20 subclasses.logger.disable("bfabric")at import), including the version banner previously logged on every client construction. The CLI re-enables it insetup_script_logging; other apps opt in withlogger.enable("bfabric").issubclassladder with adict[type, resolver]registry, an exhaustiveness check validated against theInputSpecTypeunion at construction, and an MRO-walk fallback. Drops dead spec surface (resolve_filenamestubs, unusedBfabricAnnotationSpecField) and renamesbfabric_resource_dataset.pyto the_specconvention.uv-based commands raise a clear error when theuvbinary is missing on PATH instead of passingNoneinto a subprocess call.Non-obvious decisions
logger.disable/enablefixes it properly. Note this makes all bfabric loguru logging opt-in for library/server consumers, not just the banner.reportUnannotatedClassAttributeignore.issubclass-based dispatch (via the MRO-walk fallback) to stay correct ifBfabricAnnotationSpeclater becomes a union.Testing: full
pytestforbfabric(748),bfabric_scripts(66),bfabric_app_runner(319) pass;basedpyrightclean for all three, no baseline drift;ruffclean. Pre-existingdispatch_resource_flowfailures (Python 3.14 / pandera) are unrelated — identical on cleanorigin/main.🤖 Prepared with assistance from Claude Opus 4.8 via Claude Code.