chore: code-review quick-wins cleanup (core, scripts, app-runner) - #557
Draft
leoschwarz wants to merge 10 commits into
Draft
chore: code-review quick-wins cleanup (core, scripts, app-runner)#557leoschwarz wants to merge 10 commits into
leoschwarz wants to merge 10 commits into
Conversation
Replace all 16 deprecated Entity.find/find_all/find_by (FindMixin) read-path
call sites in bfabric_app_runner with the modern client.reader API
(read_id/read_ids/query_one). Behavior-preserving:
- find(id) -> reader.read_id(endpoint, id, expected_type=...)
- find_by(obj) -> reader.query_one(endpoint, obj, expected_type=...)
(callers only used the first/truthiness of the result)
- find_all(ids) -> reader.read_ids(endpoint, ids, expected_type=...)
read_ids is URI-keyed and includes None for misses, whereas find_all was
int-keyed and dropped misses. So:
* .values() consumers now filter out None
* id-indexed consumers re-key uri.components.entity_id -> entity and drop
None, preserving the prior "missing id -> KeyError" behavior
The four possibly-None accesses that were previously grandfathered in the
basedpyright baseline (execution.dataset arg, dataset.to_polars(),
registration.workunit_id/storage_id) are now handled with targeted
# pyright: ignore comments; the baseline shrinks accordingly (no new
suppressions).
Tests updated to mock the client.reader path (real EntityUri keys for
read_ids); added focused coverage for the re-key/None-drop paths in the
dataset, resource, and resource-flow resolvers.
…pers Deduplicate the 're-key EntityReader results by id / drop not-found' transform that FindMixin.find_all/find_by performed inline into two reusable free functions in bfabric.entities.core.reader_utils. FindMixin now delegates to entities_by_id, and the app-runner FindMixin-migration sites use the helpers instead of inline dict-comprehensions, collapsing the four bulk read_ids sites to one line each. No behavior change. The basedpyright baseline shrinks by one entry (find_by's grandfathered reportReturnType is now fixed via a localized cast).
read_id/read_ids/query/query_one now accept an entity class in place of the endpoint string (e.g. client.reader.read_id(Resource, id)), inferring both the endpoint and the result type via import_entity.entity_type_of (the class->string inverse of import_entity, keyed on the lowercase-class-name convention). The string form (with optional expected_type) still works. Adopt the class form across bfabric_app_runner + a few core sites, dropping the redundant endpoint string; narrow the read-only ResolveBfabricDatasetSpecs and ResolveBfabricResourceDatasetSpecs to take an EntityReader instead of the full Bfabric client.
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
Add a 'Production code conventions' section to AGENTS.md (entity access, client construction, exceptions, logging) and condense the docstring-default rule. Apply the logging/exception conventions to three core sites: print() diagnostics -> logger.warning() (bfabric.py, multi_query.py) and bare RuntimeError -> BfabricRequestError (result_container.py). Also modernize a pre-existing isinstance tuple to X | Y in multi_query.py (ruff UP038).
…ords
bfabric now calls logger.disable("bfabric") at import (loguru's library convention),
which filters its records before any sink -- including logot's LoguruCapturer. Because
enable/disable is global process state, logot.assert_logged only passed when a random
test order happened to run a setup_script_logging caller first, so CI failed
intermittently across resolution jobs. Add an autouse fixture that enables it per test.
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.